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

RFC-160 Use ES6 in govuk_publishing_components #160

Closed
wants to merge 1 commit into from

Conversation

patrickpatrickpatrick
Copy link

@patrickpatrickpatrick patrickpatrickpatrick commented May 30, 2023

RFC-160 Use ES6 Modules in govuk_publishing_components

Rendered version

Relevant Trello Card

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

This is fantastic @patrickpatrickpatrick - stellar job. I'm really excited to see it.

I've made a few suggestions, the only thing I think that would be a good addition to the proposal would be to have a section on the route to convert things - i.e would we do it one file at a time with releases, or convert everything before a release?

@patrickpatrickpatrick patrickpatrickpatrick marked this pull request as ready for review July 18, 2023 15:28
@patrickpatrickpatrick patrickpatrickpatrick changed the title RFC-160 Use ES6 Modules in govuk_publishing_components RFC-160 Use ES6 in govuk_publishing_components Jul 18, 2023
```

```
window.GOVUK.GemComponent.init(exampleElement);

Choose a reason for hiding this comment

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

Thanks for the detailed talk this morning @patrickpatrickpatrick 😊

As this is similar to our window.GOVUKFrontend global I'll add some of our findings here

We realised a while back that it was the Rollup UMD bundle that added globals for:

window.GOVUKFrontend.Accordion
window.GOVUKFrontend.CharacterCount
window.GOVUKFrontend.Checkboxes
window.GOVUKFrontend.Radios
// etc

Whilst we're still going to publish UMD bundles in govuk-frontend@5 we'd prefer if teams used ES6 module import/export syntax too, rather than put UMD bundles in a <script type="module"> wrapper

Before

Rollup UMD bundles automatically add window globals

<script src="/govuk-frontend.min.js"></script>
<script>
  window.GOVUKFrontend.initAll()
</script>

After

Rollup ES bundles use ES6 import/export syntax

<script type="module">
  import { initAll } from '/govuk-frontend.min.js'
  initAll()
</script>

Might be a while before you can move away from window globals

But by transpiling ES6 to ES5 (using Bublé) you'd lose access to the good stuff™ like exports 😊

Choose a reason for hiding this comment

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

I should add that ES module downloads will be deferred by default

So we'd recommend they still be "preloaded" with a script tag before import

<script type="module" src="/govuk-frontend.min.js"></script>

Or using modulepreload hints where supported

<link rel="modulepreload" href="/govuk-frontend.min.js">

Copy link
Author

@patrickpatrickpatrick patrickpatrickpatrick Jul 18, 2023

Choose a reason for hiding this comment

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

With regards to transpiling at all, this is because frontend rendering applications don't have support for using ES6 modules as they are using sprockets and asset pipeline. If we were to make the significant change (moving the Javascript outside of the gem entirely) then this would require all the frontend rendering applications to be updated to using bundlers so we could actually make use of modules and actual ES6 features in Rails applications. My thinking of this RFC was to make govuk_publishing_components compatible with ES6 while causing the least disruption to our frontend applications.

Choose a reason for hiding this comment

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

Definitely. Mainly commenting to show what's coming in future

Might be a while before you can move away from window globals

But hopefully can avoid transpiling to ES5 anything inside ES2015 <script type="module">

Choose a reason for hiding this comment

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

That would make sense actually, since it wouldn't run without polyfill anyway we could just strip out the transpiling step for components. Whether to include transpiling at all depends on if we just leave analytics as they are (which I think we could, unless we want to transpire them but that could start to introducing polyfill for them which I assume would want to be avoided).

This would mean rethinking minification as Uglify is only compatible with ES5. There are alternatives out there (https://github.com/ahorek/terser-ruby) we could use so it's not a big issue. Would just mean integration would be more complex as we'd have to update the frontend rendering applications simultaneously with the changes to govuk_publishing_components.

Choose a reason for hiding this comment

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

Yeah, if we're running Babel on GOV.UK Frontend components you can hopefully use our code as-is

With UglifyJS, we saw aggressive inlining break polyfills and needed careful compatibility workarounds

We switched to Terser eventually:

},
plugins: [
resolve(),
buble(),

Choose a reason for hiding this comment

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

With Bublé in maintenance mode, see if the GOV.UK Frontend Babel integration is helpful?

For example we found Babel handled Safari 10 gotchas with block scoping and template strings

Using Browserslist config file you could:

  1. Target modern browsers for components in ES6 modules
  2. Target legacy browsers for analytics in UMD bundles

These targets can be understood by Babel's babel.config.js

Choose a reason for hiding this comment

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

This sounds like a more sensible way of doing things than using Bublé! In practice I will definitely use this!

.then(files => files.map(file => defaultConfig(file)))
```

With Rollup we can utilise a transpiler ([I used Bublé](https://buble.surge.sh/)) to convert our ES6 code into ES5. This means we can still have analytics run on IE11 and use [Uglify to minify our Javascript](https://github.com/privatenumber/minification-benchmarks#-results). Setting the output of the Javascript to [UMD](https://github.com/umdjs/umd/blob/master/templates/commonjsStrict.js) with a name of 'GOVUK.Modules.ComponentName' results in the component being [assigned to the window object that components are currently assigned to](https://docs.publishing.service.gov.uk/repos/govuk_publishing_components/javascript-modules.html#module-structure). [I also considered an alternative method in the appendix](#alternate-method-of-initialising-components).

Choose a reason for hiding this comment

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

Just that snag from this morning's dev catch up regarding the next GOV.UK Frontend release

i.e. By targeting <script type="module"> we safely removed IE11 polyfills

So take care transpiling to ES5 for legacy <script> as our GOV.UK Frontend components will (likely) crash on IE11 or not run at all. Sounds like a good plan for analytics code though

Note that ES modules behave as <script defer> by default so you may see issues executing code in the order you're used to

@patrickpatrickpatrick
Copy link
Author

Just an additional point I forgot to add; the proposed changes in the intermediary change would mean that we should move the cookie banner component to the analytics folder and out of the components folder (as we need it to run on older browsers).


### The future of IE11 support on GOV.UK

To support Internet Explorer 11 (IE11) all Javascript in govuk_publishing_components is written in the ES5 standard. This is because IE11 does not support ES6. However lately in the GOV.UK Frontend Community there have been calls to drop JS support for IE11. As of June 2022, [Microsoft no longer supports IE11](https://learn.microsoft.com/en-us/lifecycle/products/internet-explorer-11). From 24th of April to the 21st of May, the number of users that accessed GOV.UK using IE11 was 26,205. This is 0.067% of all users (38,821,598) who accessed GOV.UK during that time. The concensus in the community is that we should eventually drop support for IE11 but the timeline for this has yet to be established. Part of the complexity is that we would still want analytics to work for the not insigificant number of IE11 users.
Copy link
Member

Choose a reason for hiding this comment

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

With regards to Analytics and IE11, @dnkrj and I had an interesting chat about tracking analytics without script for old browsers with our admin tools.

It made me wonder that, given our number of users on old browsers is really small, we really don't need to have the full rich suite of analytics for them - we just need another analytics information to tell us how small that proportion of users is so that we can make the right technology choices. With that considered I wondered whether we could still have analytics code be ES6 (and thus avoid having two different browser support approaches) and then have a simple fallback so old browsers get basic analytics like page views.

Maybe something like:

<script nomodule>
   var iframe = document.createElement("iframe")
   iframe.src = "https://www.googletagmanager.com/ns.html?id=blah"
   document.body.appendChild(iframe)
</script>


[As well as the ability to test the support of a script, there are also performance benefits to individual loading of Javascript. In the performance tests in the appendix, I examine the differences in page load time and asset size if we were to switch to individually loaded Javascript](#performance-tests-of-Individual-javascript-loading).

### Significant change: Move all Javascript to a separate npm module
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm concerned about separating the JS out into a separate NPM package. You've highlighted some of the problems with this already (maintaining a separate package, how would local development work) but I think it's worth clarifying further.

Local development for frontend devs is already pretty complicated at times. For example (and I need to do this frequently) running a local application pointed at a local static, both of them pointing at a local version of govuk_publishing_components. This can be further complicated by any of those local apps randomly caching asset files, which leads to a lot of time wasted hunting through to delete them, or rebuild or restart applications. Adding another layer into this (a local version of all the JS for govuk_publishing_components) is going to make things more complicated.

It's also worth thinking about the process for making and deploying a change.

  • make change to a component in govuk_publishing_components (1 PR)
  • make change to component JS in JS package (+1 PR)
  • make change in application and static to use component change (+2 PRs)
  • make new version of JS package and new version of govuk_publishing_components to include it (+2 PRs)
  • install new version of govuk_publishing_components in static and applications (+2 PRs, possibly more)

We're looking at potentially 8 pull requests for a single change, and even more if that change turns out to have an unforeseen bug.

We also have a problem (currently unsolved) in govuk_publishing_components of JS test files having to contain their own copy of the HTML that they operate on. If the JS and the component template are modified but the HTML in the test is not, it is possible to introduce errors into the code but still have the tests pass (again this has happened several times). Separating the JS and JS tests from the components they operate on still further by moving them out of govuk_publishing_components removes any chance of solving this problem.

I think all of this might come down to the question of whether separating out the JS into a separate package is something we should do, or something we have to do.

If we have to do it then we have to, but this seems to be based on an assumption about the future of Rails that hasn't been confirmed yet. I'd also be interested to investigate if there are other approaches that could work for us even if that's what Rails decides.

As you mentioned, this issue might be worth a separate RFC.

Choose a reason for hiding this comment

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

Can you keep the npm package in the same repo?

We use npm workspaces on GOV.UK Frontend with all our tools and review app as separate packages:

package.json

"workspaces": [
  ".github/workflows/scripts",
  "packages/govuk-frontend",
  "packages/govuk-frontend-review",
  "shared/*",
  "docs/examples/*"
],
  1. Publishable packages in ./packages
  2. Internal-only packages in ./shared

Makes sure things like import 'govuk-frontend' hit the local package, not the registry, so no publishing needed. You'll notice when running npm install they'll be symbolically linked into node_modules too

But you get other npm workspaces features like:

Build all workspaces

npm run build --workspaces

Build single workspce

npm run build --workspace govuk-frontend

Copy link
Member

Choose a reason for hiding this comment

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

Just catching up on this RFC (super late) and thought I should add a comment here as I imagine this suggestion was a misunderstanding based on a suggestion I made.

The idea of using a separate repo was the misunderstanding, that should not have been a considered option. It should have been exactly what @colinrotherham described where the same repo publishes both a gem and an npm module. This is what Rails core gems do (for example: https://github.com/rails/rails/tree/main/activestorage).

Not that doing this is without headaches, but vastly less headaches than an idea of two separate codebases.


### The future of IE11 support on GOV.UK

To support Internet Explorer 11 (IE11) all Javascript in govuk_publishing_components is written in the ES5 standard. This is because IE11 does not support ES6. However lately in the GOV.UK Frontend Community there have been calls to drop JS support for IE11. As of June 2022, [Microsoft no longer supports IE11](https://learn.microsoft.com/en-us/lifecycle/products/internet-explorer-11). From 24th of April to the 21st of May, the number of users that accessed GOV.UK using IE11 was 26,205. This is 0.067% of all users (38,821,598) who accessed GOV.UK during that time. The concensus in the community is that we should eventually drop support for IE11 but the timeline for this has yet to be established. Part of the complexity is that we would still want analytics to work for the not insigificant number of IE11 users.
Copy link
Contributor

Choose a reason for hiding this comment

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

Data point: Between 10th August and 10th September '23 there were 18,841 users of IE, 18,704 of those on IE11. The next major release of the design system will drop js support for IE and is expected before the end of the year. The only pragmatic approach for GOV.UK to take is to fall in line for components inherited from the design system at which point supporting js for IE elsewhere becomes questionable in terms of effort:benefit ratio. The exceptions to this would be cookie acceptance and tracking.

@kevindew
Copy link
Member

kevindew commented May 1, 2024

As I don't think we've seen any further activity on this in over 6 months, it seems prudent to close this now and resurrect it should it be continued further.

@kevindew kevindew closed this May 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants