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

Default to ES modules with single Rollup config #3726

Merged
merged 13 commits into from
Jul 5, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented May 31, 2023

This PR consolidates all govuk-frontend npm package JavaScript into the govuk directory

We use Rollup to build JavaScript into the following formats + extensions:

  • ECMAScript (ES) modules as *.mjs
  • ECMAScript (ES) module bundles as *.bundle.mjs
  • Universal Module Definition (UMD) bundles as *.bundle.js

But like we do for our GitHub release zip files also add a "ready to go" minified bundle for:

  • ECMAScript (ES) module bundle minified as govuk-frontend.min.js

CommonJS compatibility

We've felt the pain of "ESM only" packages recently and know that CommonJS compatibility is important for both Node.js require('govuk-frontend') package resolution and for test runners that don't support ES modules yet

By dropping CommonJS and going "ESM only" we'd hit issues such as:

  1. Jest ECMAScript modules support needing --experimental-vm-modules
  2. Mocha current limitations like --watch mode only working with CommonJS

Although more complex workarounds like Babel compiling ES modules to CommonJS can work instead

Legacy bundlers

Some bundlers like Browserify support require('govuk-frontend') but may need AMD not CommonJS format. Similarly the unpkg CDN falls back to expect a UMD bundle in package.json "main"

Package layout

dist
└── govuk
    ├── govuk-frontend.min.js         # ECMAScript (ES) module bundle, minified
    │
    ├── all.mjs                       # ECMAScript (ES) module
    ├── all.bundle.mjs                # ECMAScript (ES) module bundle
    ├── all.bundle.js                 # Universal Module Definition (UMD) bundle
    │
    └── components
        ├── accordion
        │   ├── accordion.mjs         # ECMAScript (ES) module
        │   ├── accordion.bundle.mjs  # ECMAScript (ES) module bundle
        │   └── accordion.bundle.js   # Universal Module Definition (UMD) bundle
        │
        └── button
            ├── button.mjs            # ECMAScript (ES) module
            ├── button.bundle.mjs     # ECMAScript (ES) module bundle
            └── button.bundle.js      # Universal Module Definition (UMD) bundle

Script includes

But by adding "ready to go" bundles all.bundle.js (UMD) and all.bundle.mjs (ES module) we maintain non-bundler compatibility even when importing JavaScript directly (see README.md)

✅ Rails Asset Pipeline using //= require to concatenate UMD all.bundle.js
✅ Browsers using dynamic await import() to load ES module all.bundle.mjs
✅ Browsers using <script type="module"> to load ES module all.bundle.mjs

<script src="/javascripts/all.bundle.js"></script>
<script>window.GOVUKFrontend.initAll()</script>
<script type="module">
  import { initAll } from '/javascripts/all.bundle.mjs'
  initAll()
</script>

Note: Only govuk-frontend.min.js is minified, unlike other bundles

Window globals

What do we do about UMD-only window globals like window.GOVUKFrontend.Accordion?

Regarding window globals we'll continue to support UMD bundles in v5 but will deprecate them in a future release

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 May 31, 2023 19:40 Inactive
@colinrotherham colinrotherham requested a review from a team as a code owner May 31, 2023 20:02
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 May 31, 2023 20:03 Inactive
@colinrotherham colinrotherham changed the base branch from main to build-default May 31, 2023 20:57
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 08:46 Inactive
@colinrotherham colinrotherham force-pushed the build-default branch 2 times, most recently from 55f3c63 to 596b797 Compare June 1, 2023 08:56
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 08:58 Inactive
Base automatically changed from build-default to main June 1, 2023 14:11
@colinrotherham colinrotherham changed the base branch from main to rollup-configs June 1, 2023 14:43
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 14:43 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 15:00 Inactive
Copy link
Member

@romaricpascal romaricpascal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That looks promising and super helpful for discussing #3721.

Regarding the output, so we're clear on what our options are, does that mean we have complete flexibility on:

  • the format, between esm, cjs, umd and iife via the format option
  • whether the imported modules are bundled in the resulting file or left as import statements via the preserveModules option

I also like that we could move to a unique tree thanks to the ./* path in the exports (more related to #3482 that part).

@colinrotherham
Copy link
Contributor Author

Thanks @romaricpascal

Yeah it's primarily to address this "modules versus bundles" thread which caught us out

Historically we've bundled every JavaScript file separately into UMD, but now we can pick any module format and even CommonJS bundlers should be able to do tree-shaking

@colinrotherham colinrotherham force-pushed the rollup-configs branch 2 times, most recently from fb27669 to 36ee87a Compare June 1, 2023 19:31
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 19:33 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 June 1, 2023 19:40 Inactive
Base automatically changed from rollup-configs to main June 1, 2023 19:44
@36degrees
Copy link
Contributor

Just remove the version number, leaving govuk-frontend.min.js?

With the namespace directory maaaybe govuk/govuk-frontend.min.js govuk/frontend.min.js??

I reckon govuk-frontend.min.js just to be really clear, if the file is copied or routed without keeping the directory name intact it'd still be clear what was being imported if you see it in a script tag.

I don't actually remember what we said we wanted to do with all of the all.* files…

We just had too many of them! Narrowed down a lot since then:

all.cjs
all.mjs
all.bundle.js
all.bundle.mjs
all.bundle.min.js
all.bundle.min.mjsgovuk-frontend.min.js

Would it be more idiomatic to go with index.*? Otherwise I think we're good, does seem a lot more manageable with the other changes.

@colinrotherham
Copy link
Contributor Author

Would it be more idiomatic to go with index.*? Otherwise I think we're good, does seem a lot more manageable with the other changes.

@36degrees Yeah let's leave it for another PR

Must have been intentional at some point and matches Sass all.scss too

Copy link
Contributor

@36degrees 36degrees left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience as we worked through all of this @colinrotherham 🙌🏻

@colinrotherham
Copy link
Contributor Author

colinrotherham commented Jul 5, 2023

Ready for review again Beat me to it thanks @36degrees

Last push removed the version suffix from the "browser ready" minified bundle:

import { initAll } from '/javascripts/govuk-frontend.min.js'

I've updated our decision document to reflect this:

Base automatically changed from browserslist-support to main July 5, 2023 16:04
* ECMAScript (ES) modules as `*.mjs`
* ECMAScript (ES) module bundles as `*.bundle.mjs`
* Universal Module Definition (UMD) bundles as `*.bundle.js`

But like we do for our GitHub release zip files also add a "ready to go" minified bundle for:

* ECMAScript (ES) module bundle minified as `govuk-frontend.min.js`

(Except unlike our GitHub release we don’t add a version number)
See Node.js documentation for conditional exports:
https://nodejs.org/api/packages.html#conditional-exports

>Providing a "default" condition ensures that any unknown JS environments are able to use this universal implementation, which helps avoid these JS environments from having to pretend to be existing environments in order to support packages with conditional exports.
We only import `common/index.mjs` but kept this module to avoid breaking changes in v4
We already provide ES modules for all our source files so don’t need to produce additional bundles for every helper and utility too
We no longer need a separate Rollup config for UMD
This is in preparation for the switch to ES modules as our default GitHub release flavour

It ensures that export names (Accordion, Button, initAll, version etc) are maintained within the bundle, not just at export time
Alongside our UMD bundles we now have ES module bundles for completeness

Although they’re not minified, browsers can now `await import()` ES module bundles without having to resolve and download imported child modules (including potentially `node_modules` for polyfills)
This runs the Rollup build for our GitHub release JavaScript `govuk-frontend-{{ version }}.min.js` into package too but without the version number
Rollup build is not necessary when `govuk-frontend.min.js` is already built
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3726 July 5, 2023 16:07 Inactive
@colinrotherham
Copy link
Contributor Author

We'll update our README.md Importing JavaScript advice in another PR:

You can include Javascript for all components either by copying the all.bundle.js from node_modules/govuk-frontend/dist/govuk/ into your application or referencing the file directly:

<script type="module" src="<path-to-govuk-frontend-all-file>/all.bundle.js"></script>

☝️ It's still using the UMD + window globals approach but via <script type="module">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update how we load JavaScript in browser
4 participants