Skip to content
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

Cannot import standard ES6 modules from npm #711

Closed
8 tasks done
Tracked by #1389
NickColley opened this issue May 22, 2018 · 26 comments · Fixed by #2586
Closed
8 tasks done
Tracked by #1389

Cannot import standard ES6 modules from npm #711

NickColley opened this issue May 22, 2018 · 26 comments · Fixed by #2586
Assignees
Labels
feature request User requests a new feature javascript
Milestone

Comments

@NickColley
Copy link
Contributor

NickColley commented May 22, 2018

What

At the moment if you try to import using our documentation using a standard ES6 bundler such as Rollup it will fail.

This is because we ship our bundles as UMD, which is great for browser and CommonJs support but not ES6 Modules (better for build pipelines).

This means our current guidance around rollup is misleading and only works when compiling from source.

We could consider adding more real world examples that import the package directly to ensure what we recommend is tested.

I'm not sure if we should be publishing these as .mjs files, which seems to be what Node.js is going towards.

Why

Some users rely on getting modules directly from /src, which we don't recommend and don't want to encourage. Many of these users say they're running outdated versions. We should make it easier for them to import as standard ES6 modules.

Done when

  • Test including modules directly from /src
  • Research into type: module in the package.json
  • Test in some dummy apps/services
  • Automated testing
  • Implement decided approach
  • Do a pre-release and share with the community to make sure this solves the problems (try and reach out to those we know are going directly to /src)
  • Documentation
  • Raised a PR for using this in the Design System
NickColley added a commit to alphagov/govuk-design-system that referenced this issue May 22, 2018
Add Rollup plugins, includes commonjs plugins since GOV.UK Frontend is currently shipped as UMD.

See alphagov/govuk-frontend#711 for more details.
NickColley added a commit to alphagov/govuk-design-system that referenced this issue May 23, 2018
Add Rollup plugins, includes commonjs plugins since GOV.UK Frontend is currently shipped as UMD.

See alphagov/govuk-frontend#711 for more details.
@NickColley NickColley added the feature request User requests a new feature label Jun 5, 2018
@matthewblewitt
Copy link

Do you lose tree shaking by using 'rollup-plugin-commonjs'/UMD? I'm having to useimport Button from 'govuk-frontend/components/button/button'; rather than import { Button } from 'govuk-frontend' to only get the code I want.

@NickColley
Copy link
Contributor Author

Yeah I believe that's the only way currently to achieve what you'd get for free with tree shaking.

@NickColley
Copy link
Contributor Author

Would be good to also think about how polyfills would be treeshaking as part of this work: #710

@NickColley
Copy link
Contributor Author

A possible way to test tree shaking: https://twitter.com/Rich_Harris/status/1031220091940204546?s=17

@NickColley
Copy link
Contributor Author

@NickColley
Copy link
Contributor Author

@gunjam
Copy link
Contributor

gunjam commented Mar 5, 2021

In our project we use very few govuk-frontend js modules: Button, Radios, Checkboxes, ErrorSummary, that's about it. So to save on the bundle size I made a custom all.js file that only imports the components I need. The other reason we do this is that we have some custom components which use the same polyfills (and one that reuses Button) which can also be added to the same all.js file. The tree shaking will avoid bundling the same polyfills or components twice, resulting in a single small js bundle.

Unforuntatley the only way to achieve this is to add this Github repo as a npm depency, as the unbundled source is not included in the npm package and using the individually bundled components does not take advantage of tree shaking, bloating out the total bundle size.

This also presents us with some problems:

  • The version of the Github repo is not always accurately reported by the 'out of date' check in our CI pipeline so it's always yellow with a warning
  • The Github repo package.json requires specifically node version 12 preventing us from updating to current LTS version 14 (the npm install fails)
  • govuk-frontend is already a subdepency of a DWP package meaing we're effectively installing everything twice, with the nunjucks macros being the loaded from the subdepency but the client side js bundled from the source repo. Keeping these versions the same is a faff
  • This is clearly a messy hack

So in an ideal world the unbundled component js would be included in the govuk-frontend npm package in a way in which it could be imported as an ES6 module. This could also allow serving the ES6 modules directly over HTTP2 in the future, falling back to the bundle.

I'd be happy to help on the implementation of this if an approach could be agreed.

Thanks!

@kr8n3r
Copy link

kr8n3r commented Jun 23, 2021

direction most packages are moving to is to specify type: module in package.json
Sindre Sorhus’s FAQ. for more details
alternatively ship additional govuk-frontend-esm npm package

@hannalaakso
Copy link
Member

hannalaakso commented Jan 10, 2022

I did some testing last year to verify that directly including the ES6 modules from /src in the GOV.UK Frontend npm package would work with bundler imports. I imported the component JavaScript using Parcel and Webpack from the npm package and didn't encounter any problems. See https://github.com/hannalaakso/webpack-govuk-frontend for an example.

I also saw that there was about a ~20% reduction in the size of the resulting JavaScript file, although this is a bit arbitrary since I only tested the import of two components (the accordion and the character count). I also had a bit of a dig through what the removed JS consisted of and agreed with @andymantell's assessment that it seems to be mainly made up of the duplicated polyfills.

I haven't assessed the type: module solution so we should do that as well before deciding on next steps.

@domoscargin
Copy link
Contributor

I had a quick look at using "type": "module", but didn't get far.

  • The type defaults to commonjs, though the guidance is to explicitly set this in the package.json, in case defaults change in the future. We could do this in any case.
  • This is a hard switch. It will break for anybody using a require to grab govuk-frontend. This happens even if we don't change anything about the compiled files (ie: they're all still UMD).
  • Seems like this is for a package that wants to completely switch over to ESM.
  • I had some success hackily generating two sets of files via gulp rollup using the es and umd formats, though I didn't get as far as treeshaking the polyfills. In this case we could set "type": "module" for the appropriate files.

@domoscargin
Copy link
Contributor

I've got a WIP branch with some hacky non-functional bare bones approaches that I was going to run by the team.
https://github.com/alphagov/govuk-frontend/tree/bk-create-esm-files-in-package

  1. Use rollup to bundle ESM files

Doesn't really work - we're rolling up file by file, so all the polyfill duplication still happens, and you lose the opportunity for other services to roll their own bundling.

  1. Copy the JS source files as .mjs into the same locations as the generated UMD files

Big questions here are how to correctly target the .mjs files and not the UMD files (all.mjs would probably need a slight refactor?), and which files need to be kept as is.

  1. Copy all source files to a separate folder - something like govuk-esm

The published package becomes a lot larger and there's presumably going to be a lot of namespacing issues. Could the JS files only be separated into another folder with an entry point file?

vanitabarrett pushed a commit that referenced this issue Mar 24, 2022
Playing around with esm modules in govuk-frontend.

Based on original work by @domoscargin who did the initial investigation
into different approaches for this. This commit explores option 3 more.
#711 (comment)
@vanitabarrett
Copy link
Contributor

vanitabarrett commented Apr 4, 2022

@domoscargin and I have been testing this approach with the following bundlers: Webpack; Parcel; Rollup; esbuild.

Setting sideEffects

The biggest change we’ve made since my last comment is setting sideEffects in the govuk-frontend package/package.json. I originally set this to false when first prototyping approaches to this card, for unknown reasons (most likely copied from an example without giving it much thought).

However, @domoscargin noticed when he was testing that polyfills were being ignored and weren’t being bundled. Having read https://sgom.es/posts/2020-06-15-everything-you-never-wanted-to-know-about-side-effects/, I came to the conclusion that we needed to set sideEffects to true because of the way we import polyfills - we import them and rely on them being there without having to 'call' them, which means our code has ‘side effects’. A bundler might otherwise exclude these because it thinks the imported code isn't being referenced and therefore isn't needed. I’ve gone one step further and actually set sideEffects to "govuk-esm/vendor/**” because I think these are the only JavaScript files that we use in this way.

I’ve updated the pre-release with this change and tested with all the bundlers listed above. All output JavaScript now seems to correctly include polyfills and only includes them once even if they’re imported by multiple components (tree-shaking).

common.mjs

As discussed on Slack, we've decided not to export the methods included in common.mjs in our all.mjs file. These are internal methods which were never intended to be made 'public', despite people importing them at the moment in the way we currently ship our JS as UMD probably because it's convenient and there's nothing to stop them from doing that. As this is a new feature, we're not going to include the functions in that file (nodeListForEach and generateUniqueID) as an export in our ES modules.

Stats

Original UMD bundle in package/govuk: 96.01 kB
Rollup: 55.98 kB
Webpack: 76.05 kB
Parcel: 36.14 kB (minified)
esbuild: 42.76 kB

Note: variations in file size should be down to different config setups, there might be config tweaks/changes our users can make to bring that file size down even further.

Draft govuk-frontend PR

Updated pre-release branch

Test app for Webpack; Rollup; Parcel; esbuild
Note: the commit history hasn’t been updated with the latest pre-release yet. Run npm install --save "alphagov/govuk-frontend#202c4c6f82b13dd6a06183380096c0e78933d5e0 before testing with that app.

Design System branch

Prototype Kit branch (to test backwards compatibility)

@36degrees
Copy link
Contributor

This is all looking really good to me.

I did notice a couple of things on the Design System branch (possibly because it's using an older pre-release?):

Not sure if either of these things are actually issues, but thought they were worth flagging.

@vanitabarrett
Copy link
Contributor

@36degrees Good shout on nodeListForEach - I'd noted down our decision, but forgotten to update that branch. Should be updated now in this commit.

For the other point, importing from govuk-frontend is how I'd expect someone to import. We're not defining anything new called 'govuk-frontend-esm'. Instead, the fact that we're using an import statement should be enough for the govuk-frontend package.json to "point us" to the govuk-esm directory because we've made these changes to the package/package.json file.

Does that make sense?

@vanitabarrett
Copy link
Contributor

Summary of changes

We're proposing to add a new govuk-esm directory to the shipped govuk-frontend package. This new folder will only contain ES Module javascript files (published as .mjs files), including component javascript and polyfills.

In order to support both the UMD we currently ship, and these new ESM files, we will add the following to our package/package.json file:

...
 "module": "govuk-esm/all.mjs",
  "exports": {
    "import": "./govuk-esm/all.mjs"
  },
  "sideEffects": [
    "govuk-esm/vendor/**"
  ]
...

Setting the module and exports to point to the govuk-esm folder allows a user to simply write (for example) import { Accordion } from 'govuk-frontend' without having to know that the ES Modules are stored in a separate directory - the package.json will handle the entry point depending on how govuk-frontend is being included in a project. This also means the change is backwards compatible - existing users relying on the UMD files won't need to make any changes.

Because of the way we include polyfills in our JavaScript, we also need to set sideEffects: ["govuk-esm/vendor/**"]. When we import polyfills, we rely on them being there without having to 'call' them, which means our code has "side effects". Without setting this property in the package.json, a bundler would exclude the polyfills because it thinks they aren't being used.

You can preview all the proposed changes here

@vanitabarrett
Copy link
Contributor

@andymantell @gunjam @matthewblewitt This may be of interest to you as people who have previously been active on this issue. The Design System team is working on a solution for publishing our JavaScript as ES Modules (ESM) alongside the Universal Module Definition (UMD) we currently ship. Using a bundler with ES Modules also helps reduce duplication of things like polyfills.

We're looking for your feedback on any issues you might have when upgrading your service to use this new functionality. Let us know by commenting on this Github issue, or email us at [email protected].

Tell us your thoughts by Tuesday 19th April 2022.

To update to the pre-release, run npm install --save "alphagov/govuk-frontend#202c4c6f82b13dd6a06183380096c0e78933d5e0"

To take advantage of this new functionality, you’ll need to update your JavaScript to use import statements. For example:

import { initAll } from 'govuk-frontend'
initAll()

To import specific components:

import { Accordion } from 'govuk-frontend'
var $accordion = document.querySelector('[data-module="govuk-accordion"]')
if ($accordion) {
  new Accordion($accordion).init()
}

You may also need to make changes to your JavaScript bundler configuration file, such as telling it to look out for the .mjs file extension.

vanitabarrett pushed a commit that referenced this issue Apr 8, 2022
Playing around with esm modules in govuk-frontend.

Based on original work by @domoscargin who did the initial investigation
into different approaches for this. This commit explores option 3 more.
#711 (comment)
vanitabarrett pushed a commit that referenced this issue Apr 8, 2022
Playing around with esm modules in govuk-frontend.

Based on original work by @domoscargin who did the initial investigation
into different approaches for this. This commit explores option 3 more.
#711 (comment)
@vanitabarrett vanitabarrett added this to the [NEXT] milestone Apr 12, 2022
vanitabarrett pushed a commit that referenced this issue Apr 21, 2022
Playing around with esm modules in govuk-frontend.

Based on original work by @domoscargin who did the initial investigation
into different approaches for this. This commit explores option 3 more.
#711 (comment)
@gunjam
Copy link
Contributor

gunjam commented Apr 22, 2022

Hey! Thanks so much for this work, once released this should allow me install the govuk-frontend module properly again rather than include a tagged point in the repo to get the original source. I'm gonna make a branch on our frontend project to test it out and see if these changes will fit neatly into my build.
Thanks

@gunjam
Copy link
Contributor

gunjam commented Apr 25, 2022

Hi,

So I seem to have an issue where I can't import any files directly from their file paths while exports is set in the package.json:

  "exports": {
    "import": "./govuk-esm/all.mjs"
  },

This seems to restrict the node-resolve plugin of rollup from importing anything other than that file. If I remove the exports from the package.json it works fine.

For context this is what I'm trying to do in a custom all.js (import the GOVUK components I wish to init, along with an HMRC component and some that are custom to the service):

import { nodeListForEach } from 'govuk-frontend/govuk-esm/common.mjs';
import { Button, Checkboxes, ErrorSummary, Radios, SkipLink } from 'govuk-frontend';
import TimeoutDialog from 'hmrc-frontend/hmrc/components/timeout-dialog/timeout-dialog';
import PrintButton from './js/print-button';
import SaveLink from './js/save-link';
import TimeoutButton from './js/timeout-button';
import CookieBanner from './js/cookie-banner';

Rollup cannot resolve common.mjs, or any of the polyfills which I am importing in the custom modules (eg cookie-banner) as they do not appear in the package.json exports block (I assume).

I'm working on a branch of an internal project, but I'll set up a small public repo with an example if it helps.

Thanks

@gunjam
Copy link
Contributor

gunjam commented Apr 25, 2022

I've setup a small repo with an example project, it won't build, but if you go into the node_modules folder and delete the exports property from the govuk-frontend package.json it will build fine. Of course there's probably a way to work around this, but I dunno what it is yet. A solution vaugely formed in my mind but then I immediately forgot what it was, maybe it'll come back to me tomorrow. Thanks again.

@domoscargin
Copy link
Contributor

Hey @gunjam, thanks for investigating, and taking the time to set up a nice example.

This is intended behaviour - importing from common and our polyfills wasn't something we intentionally provided, and we wouldn't encourage importing from our source code folders.

In fact, as part of our work to update our browser support and fix up some of our JavaScript, we'll likely be proposing some changes to polyfills, which would affect your project. We'll put out a more detailed proposal of our changes for feedback soon.

@gunjam
Copy link
Contributor

gunjam commented Apr 26, 2022

Oh I see! Since I was @d in this thread I assumed it was related to my previous comment.

@domoscargin
Copy link
Contributor

Hi @gunjam - you may be interested in this proposal, as it will affect polyfills: #2607

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request User requests a new feature javascript
Projects
10 participants