-
Notifications
You must be signed in to change notification settings - Fork 328
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
Fix tree-shaking when importing govuk-frontend
#4961
Conversation
govuk-frontend
📋 StatsFile sizes
Modules
View stats and visualisations on the review app Action run for 9ed92a5 |
f7f0aa8
to
8b797c4
Compare
8b797c4
to
74eb9f2
Compare
74eb9f2
to
1a113c1
Compare
1a113c1
to
dd8700f
Compare
4994bf0
to
19c9695
Compare
Any side effect affecting the global scope is encapsulated either in the components classes or initAll. By marking our package as free of side effects, WebPack drops any unused export appropriately
Ensures the documentation for running 'npm run clean' without specifying a workspace remains true
Called `build:all`, not `build` so it doesn't get run by `npm run build --workspaces` on CI (in `.github/workflows/actions/build/action.yml`). If called `build` it would run before `govuk-frontend` gets build and the build would fail.
19c9695
to
604182a
Compare
@@ -0,0 +1,6 @@ | |||
import resolve from '@rollup/plugin-node-resolve' |
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.
comment Rollup needs a plugin to resolve package from node_modules
.
outDir: 'dist/vite', | ||
assetsDir: '.', |
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.
comment By default, Vite would output in dist
and the compiled JavaScript in dist/assets
. This makes it match the dist/<BUNDLER_NAME>
convention from the other bundlers.
@@ -151,6 +151,7 @@ describe('packages/govuk-frontend/dist/', () => { | |||
'govuk-prototype-kit.config.json', | |||
'gulpfile.mjs', | |||
'package.json', | |||
'package.json.unit.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.
comment Something for another PR, but there are a couple of .mjs
files that end up in the final package when they shouldn't (see the couple of .test.mjs
and .config.mjs
files after this line). There's a *.test.*
entry in .npmignore
, but it doesn't seem to do much.
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.
note We only publish a subset of what's in package/govuk-frontend
to npm because we're filtering it using the files
field of packages/govuk-frontend/package.json
:
govuk-frontend/packages/govuk-frontend/package.json
Lines 8 to 13 in f047499
"files": [ | |
"dist", | |
"govuk-prototype-kit.config.json", | |
"package.json", | |
"README.md" | |
], |
Agree it's a little confusing as we have a combination of exclusions at build time, exclusions using .npmignore
and a list of inclusions in that files
field that all intersect with each other, but I think what we're publishing is broadly 'correct'.
Not one we need to figure out in this PR necessarily but I think we should have a think as a dev team about which bundler examples to include and maybe establish some criteria. The inclusion of vite makes sense due to it's skyrocketing popularity and rollup is useful because we use it on the website, but should we also include examples for esbuild and parcel (I appreciate the example for parcel would hypothetically be nothing)? My sources for what's popular at the moment: |
9764019
to
bfa8449
Compare
bfa8449
to
298c38d
Compare
298c38d
to
b643828
Compare
I've adjusted the bundler integration tests to also account for importing via
I've added this so that we are sure that this change, and any further changes to how we do imports, don't negatively impact users who are using The leftover question for me are firstly, how comprehensive we want this testing to be? Right now both tests are just Secondly, should we get rid of the webpack example in docs/examples/webpack? From a skim of this repo's docs, we don't reference it anywhere but we do now reference the bundler testing in our docs with this PR, which could act as a set of examples. |
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 think this is looking great – @owenatgov thanks for picking this up and adding the tests for initAll
🙌🏻
I only have a couple of suggestions, neither of which are blocking.
@@ -151,6 +151,7 @@ describe('packages/govuk-frontend/dist/', () => { | |||
'govuk-prototype-kit.config.json', | |||
'gulpfile.mjs', | |||
'package.json', | |||
'package.json.unit.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.
note We only publish a subset of what's in package/govuk-frontend
to npm because we're filtering it using the files
field of packages/govuk-frontend/package.json
:
govuk-frontend/packages/govuk-frontend/package.json
Lines 8 to 13 in f047499
"files": [ | |
"dist", | |
"govuk-prototype-kit.config.json", | |
"package.json", | |
"README.md" | |
], |
Agree it's a little confusing as we have a combination of exclusions at build time, exclusions using .npmignore
and a list of inclusions in that files
field that all intersect with each other, but I think what we're publishing is broadly 'correct'.
This ensures that any changes we make to our importing process don't negatively impact users who're importing via `initAll`
b643828
to
9ed92a5
Compare
Fix tree-shaking when importing `govuk-frontend`
This PR fixes the ability for our package to be tree-shaken by bundlers without extra configuration on the users part, allowing code exported by
govuk-frontend
but not imported by the user's code to be appropriately dropped from their compiled file.The fix
The fix consists of adding
"sideEffects": true
to ourpackage.json
file. This announces to bundlers that the files of our project are free of side effects (code that modifies JavaScript globals or the DOM) on import. In turn, this lets them know that it's safe to remove code that wouldn't be imported by the user's project.While not a field officially documented by npm, this is what Webpack and Rollup look at when deciding whether they can drop code from within
node_modules
.Tests
Unit tests
The PR adds a
package.json.unit.test.mjs
to verify the presence of that field.Integration tests
This work highlighted the need for us to at least have a little check that our library works OK with major bundlers. Without setting up a complex configuration, this PR adds integration tests with Rollup, Webpack and Vite to verify that tree-shaking works as expected with them.
This required the creation of a new npm workspace in the project:
@govuk-frontend/bundler-integrations
(in.github/workflows/bundler-integrations
as this check should primarily happen on CI). This use of a workspace is necessary so thatgovuk-frontend
is consumed as an external module by the bundlers. If we were to just run the bundlers in thegovuk-frontend
workspace, bundlers would have no trouble tree-shaking unused code as it would all be code from the same project.That workspace contains light configurations for each of the bundlers (mostly to ensure either tree-shaking is enabled when it needs explicit enabling like for Webpack and/or that minifying is disabled so we can look up the names of our components in the output and not mangled variables).
This workspace is then used on CI by the
bundlers-integration.yml
workflow, automatically called as a last step by ourtest.yml
(so we benefit from the caching of the dependencies and build).Future considerations
Freedom from side effect
The package is currently free from side effects thanks to our code only affecting the global scope from inside functions or classes. If functions are not called or classes not instanciated, nothing happens from only importing our files. This may change if we start including polyfills. At that point, we'll need to be cautious of either:
Tree-shaking in file stats
While investigating this issue, we noticed that the scripts computing the stats imports from each component's file rather than from
govuk-frontend
. We may want to expand the stats to compare the file sizes when importing a component from its file and fromgovuk-frontend
(which may highlight that some unnecessary code doesn't get tree-shaken).Closes #4957