Remove build output and .min.js from repository #772
Merged
Conversation
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
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
Primarily because installing `git` hooks are easy to forget for maintainers, whilst telling `npm` to do something as a consequence of `$ npm version <patch | minor | major>` is seamless. Also worth mentioning we what we used to do in the pre-commit hook script is now greatly simplified as we don't want to have the build output and/or the `.min.js` in git anymore. Therefore, as long as we've bumped the version number in the source code, we're basically done, after having amended that change into the git commit the `npm` CLI creates.
These changes are primarily aiming for having an acceptable developer experience when working on changes locally and want to run the test suite. By using `esm` as a required package to be executed *before* the individual test files are executed, `esm` allows CommonJS/`require()` based code to use code written with ES Modules syntax. In practise that means our existing CommonJS based test suite that is `require()`ing the source code witten in ES Modules, is seamless and totally transparent. It just works without any build or transpiling pipeline like `babel` involved. Caveat: the `esm` package only support Node.js 6.x and above. That is more than okey, as we've got continous integration to verify how our changes works on different versions of Node.js, browsers & deno. Refs https://www.npmjs.com/package/esm
Because `esm` does not support legacy versions of Node.js, at least not below Node.js 6 as of writing this, we have to avoid using that whenever tests are running on legacy versions. But now that the source code (`mustache.js`) is written in ESM syntax, how on earth are we going to run tests on these legacy versions of Node.js? We gotta run the build step first, so that we end up with a `mustache.js` file in CJS, or strictly speaking it will be UMD. That's kinda pain in the backside isn't it? Yes, but running tests on legacy versions are not meant to be done locally, but rather in CI. That means we can easily automate the flow of (1) building the source code before (2) starting the test suite. For our futureselves, if we want to stop running tests on legacy versions of Node.js; the changes introduces in this commit, could be removed completely.
Because the source code is written in ESM syntax and we cannot use the `esm` package to make `require()` ESM compatible, since that package does not support legacy versions of Node.js. Also in our usage tests in CI, we need to mimck the npm packaging behaviour where building is involved.
Noteworthy tweak is the fact that it seems `zuul` are eagerly loading
all `require('whatever-package-or-path')` it sees before pushing the
contents to Saucelabs.
That was troublesome because we don't want the `esm` package to be
loaded when running browser tests. But it was, even though we loaded
`esm` conditionally after checking the current Node.js version used.
The trick was to not use `require('esm')` but `module.require('esm')`
instead, to fool the assumed look-out for `require` somewhere inside
`zuul`'s code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TDLR; we only want the source code to be in
git.These changes effectively achieves what was raised in RFC: Remove build output from repository.
Getting here surely ended up taking more time than expected. Stumbled over a fair amount of rabbit holes a long the way, and striking the balance between maintainer experience and keeping our test suite as is w/supported Node.js & browser versions in tact, wasn't straight forward.
There's some interesting nitty gritty details in each respective commit for the curious ones, so I won't elaborate too much in the PR description. But here's the outcome with these changes in place:
In the git repository:
mustache.mjs -> mustache.jsmeaning the source file is ES modules syntaxIn the npm package:
mustache.jsto be the build output with an UMD wrapper (like before)mustache.min.jsminified version ofmustache.js(like before)mustache.mjsto have ES modules syntax (like before)In other words, unchanged from npm package consumers point of view.
Who will be affected?
Those who have been downloading mustache.js from github.com.
Alternative? unpkg.com serves many browser use cases, skypack.dev is rumoured to be a good fit for deno projects.
Closes #765