Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Rename .mjs -> .js to make it ESM and not have build output in git
Although we'll now end up with only `mustache.js` in the git repo,
being written in ESM syntax, the npm package will still be as before:

- `mustache.js`: UMD, meaning CommonJS, AMD or global scope
- `mustache.mjs`: ESM
  • Loading branch information
phillipj committed Mar 5, 2021
1 parent 9faa18e commit cc979e0
Show file tree
Hide file tree
Showing 7 changed files with 700 additions and 1,487 deletions.
2 changes: 1 addition & 1 deletion .eslintrc
Expand Up @@ -19,7 +19,7 @@
},
"overrides": [
{
"files": ["mustache.mjs"],
"files": ["mustache.js"],
"parserOptions": {
"sourceType": "module",
"ecmaVersion": 2015
Expand Down
15 changes: 0 additions & 15 deletions .github/workflows/verify.yml
Expand Up @@ -54,18 +54,3 @@ jobs:
run: |
npm install mocha@3 chai@3
npm run test-unit
build-output-sync:
runs-on: ubuntu-latest

steps:
- uses: actions/checkout@v1
- name: Setup Node.js
uses: actions/setup-node@v1
with:
node-version: 12.x
- name: Install, build and check git diff
run: |
npm ci
npm run build
git diff --quiet HEAD --
1 change: 1 addition & 0 deletions .gitignore
Expand Up @@ -9,3 +9,4 @@ npm-debug.log
.DS_Store
test/render-test-browser.js
.idea/
mustache.mjs
2 changes: 1 addition & 1 deletion hooks/pre-commit
Expand Up @@ -78,6 +78,6 @@ class Bumper
end

bumper = Bumper.new([
Source.new('mustache.mjs', /version: '([^']+)'/),
Source.new('mustache.js', /version: '([^']+)'/),
])
bumper.start

6 comments on commit cc979e0

@hammond13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may just not understand something, but was the impact on cdnjs.com taken into consideration with this change? I tried to use 4.2.0 and got javascript errors:
https://cdnjs.com/libraries/mustache.js

@phillipj
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @hammond13,

thanks for reporting!

No, cdnjs.com was not considered specifically. I don't know the details of cdnjs.com, but I do notice that it seems to be serving (my Chrome browser at least) the EcmaScript Modules version by default, rather than the UMD version. Is that a decision cdnjs.com takes because my browser supports ESM my default? 🤔

Regardless of dynamic behaviour from cdnjs.com or not, I can easily see why that change would cause havoc when mustache.js is used from cdnjs.com.

As a comparison, I personally feel would expect what is seen from unpkg.com to be more intuitive, meaning UMD by default (as mustache.js npm package declares): https://unpkg.com/mustache@4.2.0/mustache.js

@hammond13
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, @phillipj

I think it's autoupdating mustache.js based on this configuration file: https://github.com/cdnjs/packages/blob/master/packages/m/mustache.js.json

I think it's just serving the version of mustache.js as-is from the github repository.

I've never used npm. I guess it's about time I wrap my old-school (and just plain old) web dev mind around that :-)

  • David

@phillipj
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh that's interesting, thanks for sharing!

It feels weird if there's no way for cdnjs.com to use the npm package contents, instead of the raw github.com contents. Any chance you've stumbled upon documentation on how that .json format of their works?

@phillipj
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Answering my own question, I found this in their contributing guide which looks relevant: cdnjs/packages CONTRIBUTING.md#npm-based-auto-update-example

@phillipj
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For future ref: cdnjs/packages#761

Please sign in to comment.