Transition source code to be an ES module #728

phillipj · 2019-11-06T21:23:01Z

TDLR; allow Node.js and Deno to use this package as a native ECMAScript module.

Up until now, we've very deliberately held back on introducing any kind of build step to this project. Personally think that's been a wise decision as the benefits up doing so has not been obvious in terms of the complexity such a build step usually introduces in a project. Keeping everything in one .js file in good old fashioned ES3 has worked out really well so far.

That changes with what's introduced in this PR.

Why on earth should we suddenly change what has worked well all these years?? To me it's about following along with how both Node.js & JavaScript evolves and reacting to new related runtimes emerging, like Deno is a great example of.

Node.js, Deno and several modern browsers has built-in ES module support. Hence it's only natural to consider following along and make this package an ES module, rather than holding back just for the sake of not introducing a build step for the purity it.

Ofcourse backwards compatibility with non-ES module systems are kept as is, this is where a build step comes into play. The source code is now put inside the mustache.mjs file, and mustache.js | mustache.min.js is the build step output.

Tried my best creating a sensible commit history with some in-depth reasoning for each of them. Hopefully they'll shed some more light on the rationale and be of some help going forward (if we decide to merge this).

Missing pieces being worked on:

~~[ ] type: module in package.json as described in Node.js ECMAScript Modules docs~~
make CI module-systems test for Node.js using it as an ES module

UPDATE:

After reading the details about Node.js' behaviour when type: module is set, that does not seem appropriate for us. It would result in making Node.js interpret mustache.js as an ES module because it's set as the main file. That in combination with us wanting to keep that file backwards compatible with CommonJS & UMD, mustache.js will have to stay non-ES Module.

Fixes #698

In an effort of moving with recent changes in Node.js and its built-in support for ES modules, we're making the main source code reside inside `mustache.mjs`. This will also allow Deno users to use this package as an ES module directly by importing `mustache.mjs`. An ES3 / UMD version of the source code is still planned to exist in `mustache.js`, but will involve a build step to transform the ES module source into a plain .js version. By renaming the file *before* changing any contents, git will recognise the file has been renamed and therefore allows us to see all historical changes to the old .js source code with the `--follow` argument: ``` $ git log --follow -- mustache.mjs ``` Without ensuring git sees this is a file rename, we will in practise loose the old source code's history, or at least make it quite weird and less intuitive to find. That's not fair to the historical contributors and in general getting hold of a well documented log of changes to a file in git, is extremely valuable in those few scenarios where it's *really* needed.

As part of transition the source code to be a proper plain ES module, we're removing the handcrafted UMD wrapper around the source code. This means the UMD wrapper will have to come from elsewhere; a build step introduced later.

phillipj · 2019-11-22T21:49:44Z

@zekth & friends; a few weeks has passed since I ran CI, and after re-running the test that verifies mustache.js works with deno now, it exploded with what seems like TypeScript errors to me;

$ deno test --allow-net=deno.land test/module-systems/deno-test.ts

...

Compile file:///home/runner/work/mustache.js/mustache.js/test/module-systems/deno-test.ts
Download https://deno.land/std/testing/mod.ts
Download https://deno.land/std/testing/asserts.ts
Download https://deno.land/std/fmt/colors.ts
Download https://deno.land/std/testing/diff.ts
Download https://deno.land/std/testing/format.ts
error TS2371: A parameter initializer is only allowed in a function or constructor implementation.

► https://deno.land/std/testing/asserts.ts:132:39

132 export function assert(expr: unknown, msg = ""): asserts expr {
                                          ~~~~~~~~

error TS2304: Cannot find name 'asserts'.

...

https://github.com/janl/mustache.js/pull/728/checks?check_run_id=316803283#step:5:44

Any thoughts what might be causing this? Would be great to know we don't merge an unstable test that might fail if it hasn't run for a while..

I wouldn't be surprised at all if it's because I'm doing something wrong, either in the related deno CI setup or executing the test via deno.

test/module-systems/deno-test.ts

phillipj · 2019-11-23T11:40:01Z

Ahh! I'm assuming what I'm doing is not best practice in terms of importing without a hint of version number anywhere. Should it be part og the import or some external file, like package.json for npm projects?

…

On Sat, 23 Nov 2019 at 10:25, Vincent LE GOFF ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In test/module-systems/deno-test.ts <#728 (comment)>: > @@ -0,0 +1,17 @@ +import { test } from "https://deno.land/std/testing/mod.ts"; +import { assertEquals } from "https://deno.land/std/testing/asserts.ts"; You're not aiming the version in here. CI is running using 0.21.0 let's try using ***@***.***/ here? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728?email_source=notifications&email_token=AAJMWE2EIZSAIJLKXETKHDLQVDZJTA5CNFSM4JJ5P3B2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCMX5O7A#pullrequestreview-321902460>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMWE6PCMJD4DT73HJGYVDQVDZJTANCNFSM4JJ5P3BQ> .

zekth · 2019-11-23T11:47:48Z

By this way you import the Latest version which is not recommended because your runtime and your imports may have incompatibility. Like using node 8 with node std lib of node 12

phillipj · 2019-11-23T11:55:07Z

Yupp, that struck me too with your first answer. So currently this is always controlled by specifying the version in each and every import in a project, or is there some kind module management alternative as well that I should be aware of?

…

On Sat, 23 Nov 2019 at 12:47, Vincent LE GOFF ***@***.***> wrote: By this way you import the Latest version which is not recommended because your runtime and your imports may have incompatibility. Like using node 8 with node std lib of node 12 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728?email_source=notifications&email_token=AAJMWE3WGC2XFE3EJOYEKJLQVEJ6JA5CNFSM4JJ5P3B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE7TQCY#issuecomment-557791243>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMWEYSDDBQCVPBWAU2423QVEJ6JANCNFSM4JJ5P3BQ> .

zekth · 2019-11-23T11:58:08Z

Yes you can use import maps: denoland/deno#3268 (comment)

phillipj · 2019-11-23T12:07:15Z

Thanks, much appreciate your quick replies on this! I'll have a look at improving it later tonight.

…

On Sat, 23 Nov 2019 at 12:58, Vincent LE GOFF ***@***.***> wrote: Yes you can use import maps: denoland/deno#3268 (comment) <denoland/deno#3268 (comment)> — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#728?email_source=notifications&email_token=AAJMWEYBPWEGDCS23QXKVI3QVELFDA5CNFSM4JJ5P3B2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEE7TVNA#issuecomment-557791924>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJMWE3OG6EXBT3RFQEM3CDQVELFDANCNFSM4JJ5P3BQ> .

phillipj · 2019-11-23T21:20:14Z

🛑 found after having a Node.js test in place, with v13.2.0 that has .mjs support enabled by default: esm-usage test result.

That CI test result points to a big difference compared to how the module resolution in Node.js has worked until now; .json files cannot be imported directly. This is outlined in the Node.js / ESM / Experimental JSON Modules docs section, but doesn't help much as of writing this, because that is experimental and therefore opt-in with CLI flags. Personally would really appreciate if we wouldn't end up having to tell Node.js ES Modules users to remember to enable experimental features.

Therefore also worth consider other ways to getting the current version number into the source code. Or even ask ourselfs if having that .version property is really valuable to anyone.

Personally feel we need to sort this out before merging to master & publishing anything to npm.

zekth · 2019-11-25T09:01:29Z

Personally would really appreciate if we wouldn't end up having to tell Node.js ES Modules users to remember to enable experimental features.

Definitely agree.

Is injecting the version in the build process thinkable ? (it's a bit ugly compared to importing from package.json)

phillipj · 2019-11-25T11:26:19Z

Also considering that even if Node.js ES modules would have .json support, I noticed the docs or some GitHub issue I stumbled upon, said that named imports would not be supported. In practise that would mean that we would have to import the entire contents of package.json, which for the built output is not ideal, when the only thing we really want is the rather simple version field value.

I'm not too familiar with rollup.js to be honest, might be a very natural way of doing that as part of the build process as you suggest 🤷‍♂

zekth · 2019-11-25T14:53:31Z

I'm not too familiar with rollup.js to be honest, might be a very natural way of doing that as part of the build process as you suggest 🤷‍♂

We can use this in the build process: https://github.com/rollup/plugins/tree/master/packages/replace

phillipj · 2019-11-25T15:23:57Z

Nice!

That definitively looks like what we need to make sure old the .version field is kept for non-ES module systems. Although won't have much effect for the ones using the .mjs version 🤔 Which makes question the added value of that field to be honest.

Maybe there's some traces of the reason it was introduced in the first place if we dig into the git history / GitHub PRs.

phillipj · 2019-11-25T21:29:27Z

On second thought, we've managed to survive all these years with the .version field value being hard coded in the source file (mustache.js). It has been kept in sync by a git pre-commit hook:

mustache.js/hooks/pre-commit

Lines 27 to 32 in e52478c

    
           # get package.json version 
        
           package = JSON.parse File.read 'package.json' 
        
           @target_v = package['version'] 
        
           @bumped = false 
        
           @sources.each {|source| bump_source(source)}

Maybe we should keep that ad-hoc tool for now. ESm-ifying isn't really about refactor that .version anyways, so I realise I might have jumped that gun prematurely.

EDIT: in practise that means reverting 90aafdb.

By making the main source code an ES module placed in `mustache.mjs`, what used to be in `mustache.js` is now built based off of the ES module source inside `mustache.mjs`. This is done primarily to avoid breaking existing projects already using mustache.js, by keeping the UMD-version in `mustache.js` part of the git repository in addition to the minified version in `mustache.min.js`. After experiment with several compilers; - Babel - TypeScript - Rollup and examining their build output, [Rollup](https://rollupjs.org/) was chosen because of the UMD output it creates which closely resembles what we've historically had in the previous `mustache.js` source code. The contents of `.js | .min.js` files has been generated by running the following npm script: ``` $ npm run build ``` Also change linting w/eslint to target the `.mjs` file, since the output of rollup that ends up in the old `mustache.js` file is not sensible to do linting on.

Minor adjustments needed to make the TypeScript compiler that is built into Deno, be happy with how mustache.js' ES module source looks in terms of function parameters passed and object mutability. Refs #1

To ensure the ES module and source exposed by this package is compatible with Deno going forward. Added benefit is we get some sanity checks of the source code for free due to Deno's built-in TypeScript compiler getting angry if it sees things that does not make sense, in terms of missing function arguments and so on.

With the introduction of a build step, where we take the source in .mjs and build it into .js for non-ES Module systems, it's very important that we remember to keep those two in sync. That's what the CI job created in these changes ensure; 1. Run the build script 2. See if there are any pending git changes as a result As long as there are no pending git changes, we're all fine and dandy, or else we need to run the `npm run build` and commit the changes.

phillipj · 2019-12-04T20:02:50Z

With all known issues fixed, commits cleaned up and PR no longer marked as a draft, this is very close to getting merged.

No objections has been made and we've got a handful of tests in place verifying usage from module systems we want to officially support.

This changes the pre-commit hook that we've used for years to keep the version value found in `package.json` in sync with the one in `mustache.js`, to change `mustache.mjs` instead since that's where the source code lives now.

The git pre-commit hook we've been using the last years to keeping .version found in `package.json` in sync with the .version field exposed by the source code, did not handle version numbers often used for pre-relases like `beta | new` etc. Therefore making sure it searches for versions that also contains characters, not only numbers separated by dots. That will make sure the following is also seen as valid versions: `3.2.0-beta.0`, whereas before the `-beta` part would cause trouble.

phillipj · 2019-12-07T11:19:01Z

For the sake of testing the pre-commit in practise, v3.2.0-beta.0 has been published to npm. Tweaks had to be made to that pre-commit, so good to have that checked off before merging as well.

Will be verifying the contents of that beta version as the last frontier before pressing the o-mighty-merge button.

Sorry for being slow, I just want to be as certain as possible this won't cause havoc knowing this is a big change in terms of how this module is glued together. Quite surprised we have 1.3 million downloads a week from npm these days, that deserves respect when pushing new versions IMO 😃

phillipj · 2019-12-18T21:04:16Z

Publish to npm as v3.2.0, with updated CHANGELOG.md.

@zekth thanks for pushing this forward and being incredibly patience! Hoping the deno community will get some value of this going forward 🤞

phillipj added 2 commits November 6, 2019 21:45

phillipj mentioned this pull request Nov 6, 2019

ES Module support #698

Closed

phillipj mentioned this pull request Nov 19, 2019

Consider refactor with TypeScript? #729

Closed

zekth reviewed Nov 23, 2019

View reviewed changes

test/module-systems/deno-test.ts Outdated Show resolved Hide resolved

phillipj marked this pull request as ready for review December 4, 2019 19:41

phillipj and others added 5 commits December 4, 2019 20:42

Make mustache.mjs work with Deno

8f0496a

Minor adjustments needed to make the TypeScript compiler that is built into Deno, be happy with how mustache.js' ES module source looks in terms of function parameters passed and object mutability. Refs #1

Add CI test verifying native ES Module usage for Node.js

ba3fcff

phillipj force-pushed the esm-ify branch from df6f1b9 to ba3fcff Compare December 4, 2019 19:49

phillipj added 3 commits December 7, 2019 12:06

🚢 bump to version 3.2.0-beta.0

e0097b1

phillipj force-pushed the esm-ify branch from 4973d1b to e0097b1 Compare December 7, 2019 11:09

phillipj merged commit 492d683 into janl:master Dec 18, 2019

phillipj deleted the esm-ify branch December 18, 2019 20:51

phillipj mentioned this pull request Jan 7, 2021

RFC: Remove build output from repository #765

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transition source code to be an ES module #728

Transition source code to be an ES module #728

phillipj commented Nov 6, 2019 •

edited

Loading

phillipj commented Nov 22, 2019

phillipj commented Nov 23, 2019 via email

zekth commented Nov 23, 2019

phillipj commented Nov 23, 2019 via email

zekth commented Nov 23, 2019

phillipj commented Nov 23, 2019 via email

phillipj commented Nov 23, 2019

zekth commented Nov 25, 2019

phillipj commented Nov 25, 2019

zekth commented Nov 25, 2019

phillipj commented Nov 25, 2019

phillipj commented Nov 25, 2019 •

edited

Loading

phillipj commented Dec 4, 2019

phillipj commented Dec 7, 2019

phillipj commented Dec 18, 2019

Transition source code to be an ES module #728

Transition source code to be an ES module #728

Conversation

phillipj commented Nov 6, 2019 • edited Loading

phillipj commented Nov 22, 2019

phillipj commented Nov 23, 2019 via email

zekth commented Nov 23, 2019

phillipj commented Nov 23, 2019 via email

zekth commented Nov 23, 2019

phillipj commented Nov 23, 2019 via email

phillipj commented Nov 23, 2019

zekth commented Nov 25, 2019

phillipj commented Nov 25, 2019

zekth commented Nov 25, 2019

phillipj commented Nov 25, 2019

phillipj commented Nov 25, 2019 • edited Loading

phillipj commented Dec 4, 2019

phillipj commented Dec 7, 2019

phillipj commented Dec 18, 2019

phillipj commented Nov 6, 2019 •

edited

Loading

phillipj commented Nov 25, 2019 •

edited

Loading