-
Notifications
You must be signed in to change notification settings - Fork 334
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Compile ES modules using Rollup #3386
Conversation
tasks/build/package.mjs
Outdated
// Copy GOV.UK Frontend JavaScript (ES modules) | ||
await files.copy('**/!(*.test).mjs', { | ||
// Compile GOV.UK Frontend JavaScript (ES modules) | ||
await scripts.compile('!(*.test).mjs', { |
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.
Notice the glob pattern has the suffix **/
removed
Rollup will automatically find child import
modules for us from top-level entry scripts like:
./src/govuk/all.mjs
./src/govuk/common.mjs
./src/govuk/i18n.mjs
b8f6fde
to
d49d58f
Compare
tasks/scripts.mjs
Outdated
input: moduleSrcPath, | ||
|
||
// Preserve JavaScript ESM import statements | ||
experimentalPreserveModules: format === 'es' |
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.
Regarding the "experimental" option name, I've taken a closer look and we're good to go.
Since we're currently pinned on Rollup v0.59.4 (for IE8 { legacy: true }
) we get stable module support. But until the Rollup v1.0.0 release there were few missing features to be aware of:
- No dynamic
import()
- No multiple
input
named entries in watch mode - No chunk optimisation
- No tree shaking
We don't actually need these (yet) as we currently only copy our *.mjs
files between directories.
I've added some background info for context:
Rollup v0.57.0
The option { experimentalPreserveModules }
was added in Rollup v0.57.0
This takes us from 15 March 2018 to the last IE8 release in 28 May 2018:
rollup@0.57.0
rollup@0.57.1
rollup@0.58.0
rollup@0.58.1
rollup@0.58.2
rollup@0.59.0
rollup@0.59.1
rollup@0.59.2
rollup@0.59.3
rollup@0.59.4
Rollup v0.58.0
It was stabilised in Rollup v0.58.0 with some notable PRs:
Hoisting of deep imports for performance
(e.g. Polyfills moved into all.mjs)
rollup/rollup#2073
Fixes for "single file" builds like ours
rollup/rollup#2123
Warnings removed for tree-shaken imports (that were exported)
rollup/rollup#2124
Rollup v0.59.0
With another fix in Rollup v0.59.0
Fixes for file paths in source maps
rollup/rollup#2161
Rollup v1.0.0
The option was renamed to { preserveModules }
in Rollup v1.0.0
259203b
to
bd7a622
Compare
To track what was mentioned in the dev catch-up, the hoisting of deep imports may have impacts on tree-shaking, which need looking into. In our current package, However with the polyfills of all components being hoisted when building |
d49d58f
to
09757e7
Compare
@romaricpascal That's a great point Thankfully the hoisted imports in Below is some testing from our webpack example using only Bundling via
|
c74785a
to
28db545
Compare
09757e7
to
f0fb292
Compare
28db545
to
16ff2e8
Compare
f0fb292
to
2b1a035
Compare
16ff2e8
to
527ade8
Compare
2b1a035
to
62fc732
Compare
527ade8
to
2fb438f
Compare
62fc732
to
1ed4665
Compare
20c939d
to
09453b3
Compare
1ed4665
to
b28fa22
Compare
ebc21bf
to
d761c8c
Compare
c6944c1
to
53de370
Compare
d761c8c
to
d970378
Compare
53de370
to
ae02863
Compare
d970378
to
7b5fc1e
Compare
ae02863
to
7d0157e
Compare
7b5fc1e
to
a47ebdb
Compare
7d0157e
to
929601e
Compare
7887cab
to
69b3f08
Compare
929601e
to
f4326b9
Compare
f4326b9
to
f8a3a20
Compare
I've completed testing in Rollup with Good news, JavaScript output is identical Source map content appears to be the Rollup-transformed ES module code (with semicolons inserted etc) but this is likely GOV.UK Design System |
}) | ||
// Option 1: Rollup bundle set (multiple files) | ||
// - Module imports are preserved, not concatenated | ||
if (moduleDestPath.endsWith('.mjs')) { |
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.
This function seems to be doing 2 different jobs in two separate code paths – would it make sense to split it into two functions (one for generating a bundle and one that preserves modules) and decide which one to call in the compile
function instead?
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.
Yeah could do. Only reason I didn't is that we'll just use the new code path in rollup@3
and delete the old
In newer versions of Rollup the { preserveModules }
option can also output CJS/AMD
Is it a blocker?
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.
Gotcha, so once we've upgraded to v3 we'll be able to simplify this?
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.
Exactly. In this PR we process:
- Entry point
*.mjs
files for ESM, top level only - Every non-test
*.mjs
file for UMD, recursively with**
task.name('compile:ms', () => scripts.compile('!(*.test).mjs') // Entries all.mjs, common.mjs etc
task.name('compile:js', () => scripts.compile('**/!(*.test).mjs') // Every non-test *.mjs file
But in future we'll use entry points for both and let Rollup discover all child modules.
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.
OK, that makes sense 👍🏻 Happy to leave this for now then.
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.
We tried this in #3462 but we'd have to output CommonJS rather than the hybrid Universal Module Definition (UMD) bundles for browsers (see comment)
This PR uses Rollup to compile (not copy) ES modules into ./package/govuk-esm
It adds on to recent "Build tasks" work to unblock: