-
Notifications
You must be signed in to change notification settings - Fork 2.4k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
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
Showing
7 changed files
with
700 additions
and
1,487 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,3 +9,4 @@ npm-debug.log | |
.DS_Store | ||
test/render-test-browser.js | ||
.idea/ | ||
mustache.mjs |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
cc979e0
There was a problem hiding this comment.
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
cc979e0
There was a problem hiding this comment.
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
cc979e0
There was a problem hiding this comment.
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 :-)
cc979e0
There was a problem hiding this comment.
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?cc979e0
There was a problem hiding this comment.
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
cc979e0
There was a problem hiding this comment.
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