Leverage rollup's --config* feature for choosing bundles#477
Merged
mjc1283 merged 3 commits intooptimizely:masterfrom May 15, 2020
Merged
Leverage rollup's --config* feature for choosing bundles#477mjc1283 merged 3 commits intooptimizely:masterfrom
mjc1283 merged 3 commits intooptimizely:masterfrom
Conversation
Rollup provides a feature specifically designed for passing in config options to the rollup configuration module. That is: --config-* options. (It is documented deeply under https://rollupjs.org/guide/en/#configuration-files ) I believe this is a superior mechanism for passing options to rollup's config because they do not impact process.env. Environment variables have a very specific capability, which is that they are inherited by child processes. These are settings that quite literally "configure the environment under which a process runs". However, their use prior to this commit was an abuse in order to simply pass an _option_ to the rollup configuration that would control which bundles to build. Rollup's --config* feature is superior for this role, because the option is only intended to go so far as the configuration module itself. Thus, using them to control which bundle(s) are built does not leak extraneous information into the *ahem* environment of the build process. As a side benefit, the cli invocation is cleaner!
ESM consumers are very likely using their own bundler (rollup, webpack) and would probably prefer to consume non minified ESM entrypoint so they can minifiy it themselves with their preferred minification tools.
mjc1283
approved these changes
May 15, 2020
Contributor
mjc1283
left a comment
There was a problem hiding this comment.
LGTM. Agree that the --config option is a better approach than environment variables. Also the unminified ESM output is great to have.
jasonkarns
added a commit
to github/optimizely-javascript-sdk
that referenced
this pull request
May 20, 2020
* master: Leverage rollup's --config* feature for choosing bundles (optimizely#477)
zashraf1985
added a commit
that referenced
this pull request
Apr 11, 2022
## Background This PR is copied from [484](#484). It was contributed by @jasonkarns a couple of years ago. I just pulled his changes, merged master and resolved conflicts. ## Summary ESM entrypoints are very very likely to be consumed by bundlers, not loaded directly into browsers. Therefore it is preferable that the bundlers have access to the unminified source so that consumers can have more control over the final output. ~~This change _adds_ an ESM output bundle that is not minified with terser.~~ (#477 has merged which also adds the unminified ES bundle, so this PR now just makes the unminified bundle the `module` entrypoint.) Notably, the unminified bundle is created _in addition to_ the minified bundle; so the minified bundle is still distributed with the package (at the same output location). This way users who do actually want the minified bundle may still use it. However, the `package.json#module` entrypoint is changed to reference the unminified bundle, as this is most likely what users will want when consuming from a bundler. (And bundlers are virtually the exclusive consumers of the `module` entrypoint.)
This file contains hidden or 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
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.
Summary
Rollup provides a feature specifically designed for passing in config
options to the rollup configuration module. That is: --config-* options.
(It is documented deeply under
https://rollupjs.org/guide/en/#configuration-files )
I believe this is a superior mechanism for passing options to rollup's
config because they do not impact process.env.
Environment variables have a very specific capability, which is that
they are inherited by child processes. These are settings that quite
literally "configure the environment under which a process runs".
They are the correct tool for setting env vars that should be set
within node during compilation, but not a proper replacement for
cli "flags".
Their use prior to this commit was an abuse in order to simply
pass an option to the rollup configuration that would control which
bundles to build. Rollup's --config* feature is superior for this role,
because the option is only intended to go so far as the configuration
module itself. Thus, using them to control which bundle(s) are built
does not leak extraneous information into the ahem environment of the
build process.
As a side benefit, the cli invocation is cleaner!
Another major benefit of this is that it allows explicitly choosing a
subset of bundles to build. ie:
rollup -c --config-cjsto build all commonjs bundlesrollup -c --config-json --config-umdto build the jsonSchema bundleand the UMD bundle.
And since the flags match on substrings, cross-matrices are supported, too.
For instance, say the following bundles were available:
cjs-node,cjs-react,cjs-browser,esm-node,esm-browserIt would be possible to build all the node bundles or all the ESM bundles,
or all the cjs bundles or esm bundles.
rollup -c --config-esmrollup -c --config-cjsrollup -c --config-noderollup -c --config-browserNotably, the implementation dedupes the options, so if a given bundle
is matched by multiple flags, it's only built once.
This implementation is flexible so users may build precisely the bundles
they want, without waiting on extraneous bundles to build.
The developer ergonomics are clean so it's simple when invoking the rollup
CLI directly.
And the flags themselves are clean explicit enough that intent is "clear"
when being read from npm-scripts. (ie, "self-documenting")
A couple other quick nits in this PR:
npm's pre/post hooks keeps scripts clearly explicit as to their primary
purpose)
represent. ("config" is an overly general term. each rollup config represents
an output bundle, so that term is used instead. I mean, the entire
rollup configuration module is "config" so... that's almost meaningless)
cjshelper function is renamed (virtually every function"gets" something. prefixing a function with "get" is akin to
doFoo.this dispenses with the imperative naming and names the function
for what it returns. Because a function's name at the callsite is literally
replaced with its return value, so what it returns is what it should be
named.)
prettieron the entire config to clear up any inconsistencymany ESM consumers will be running their own rollup/webpack
bundlers, and therefore doing their own minification. Providing
an unminified ESM entrypoint makes their debugging easier.
(Last note, a portion of this PR is whitespace thanks to prettier,
so when reviewing the diff on github, disabling whitespace will make
the rendered diff smaller/cleaner.)