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

Upgrading to govuk-frontend v5 #352

Open
kr8n3r opened this issue Sep 5, 2024 · 16 comments
Open

Upgrading to govuk-frontend v5 #352

kr8n3r opened this issue Sep 5, 2024 · 16 comments

Comments

@kr8n3r
Copy link
Contributor

kr8n3r commented Sep 5, 2024

As part of maintenance and WCAG 2.2 compliance, we'd like to use the latest major version of govuk-frontend
Version 5 Javascript support is limited to this support grades https://github.com/alphagov/govuk-frontend/blob/main/docs/contributing/browser-support.md as they use ES modules
"Only browsers in grades A, B and C will run our JavaScript enhancements. We will not support our JavaScript enhancements for older browsers in grade X"

We need to decide if
a) we're going to use babel to transpile to ES5 or
b) build govuk-frontend Javascript as intended and serve a no-js experience for browsers that don't support it.

Most of the Javascript modules here are ES5 using global window so we'll need to think about potentially rewriting those to ES modules.

Additionally, downstream projects can write and include their own additions to application.js. We can expect that their javascript is likely to be ES5 and possibly using jQuery as well.

One of the options is to generate two files: ESM and non-ESM and bundle downstream into the latter. Similar to what we're doing on notify-admin for now

@huwd
Copy link
Member

huwd commented Sep 5, 2024

A concern I have is us changing the asset pipeline in a way which impacts any JS currently written in various tech docs sites around.

If we needed for force a rewrite there is dramatically raises the cost of this as a breaking change.
Any thoughts there on if we could bundle both ESM and non-ESM and then ensure that any existing JS in the doc site remains compatible during build?

If we did that then this might not even be a breaking change?

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 5, 2024

A concern I have is us changing the asset pipeline in a way which impacts any JS currently written in various tech docs sites around.

If we needed for force a rewrite there is dramatically raises the cost of this as a breaking change. Any thoughts there on if we could bundle both ESM and non-ESM and then ensure that any existing JS in the doc site remains compatible during build?

If we did that then this might not even be a breaking change?

yes do "generate two files: ESM and non-ESM and bundle downstream into the latter"

then anyone who currently adds their own javascript into the application.js like https://github.com/alphagov/govwifi-tech-docs/blob/master/source/javascripts/application.js would be able to do so.
if we need to use any govuk-frontend JS in the gem, then we'd create an ESM bundle

@huwd
Copy link
Member

huwd commented Sep 5, 2024

oops forgot to push this earlier

I've started a branch here:
https://github.com/alphagov/tech-docs-gem/tree/test-upgrade-to-5x

Tried to address all the initial build issues, but will need some help thinking through the JS from here on out.
Hopefully a useful starting point if we want to pair / work on it next week?

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 6, 2024

I'll have a look at your branch and try approaches there

@romaricpascal
Copy link
Member

romaricpascal commented Sep 6, 2024

If that's helpful for the update, GOV.UK Frontend also provides unminified UMD modules. While they're not the main route for using GOV.UK Frontend, we have them in for projects whose assets compilation do not necessarily support ESM.

The file with all the components is all.bundle.js, though if the tech_docs_gem only use specific components, best to only import those individually (each have a <COMPONENT_NAME>/<COMPONENT_NAME>.bundle.js file) to reduce the final bundle size.

We'd still recommend loading the built script with <script type="module">. This is to avoid older browsers loading scripts that they won't be able to parse as these UMD modules have modern syntax in them (const, let, class...) that older browsers will error on.

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 6, 2024

@huwd something along the lines of 39502c0
separate entry for govuk-frontend with type module (either minified file or all.mjs and we compile it with the new asset pipeline)

@huwd
Copy link
Member

huwd commented Sep 9, 2024

Ah great, I'll take a closer look at that this week - perhaps we can find that time to pair.

I'm intrigued from the commit that you say it's working for serve but not for build,
Does that assume that so long as we've already got some pre-compiled assets from old builds, then we're good?

So the last task is to ensure that assets unchanged here need to get through the build step?

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 10, 2024

the newly created govuk_frontend.js does not get generated in the build step, not sure if it's the limitation of the asset pipeline or lib setup or something else.
As said, ideally we don't want downstream users to change anything

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 10, 2024

exploration using middleman external pipeline with gulp 26f13f0

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 11, 2024

We've explored this a bit further with @huwd , summary below

Problems

Compile Sass

Sass files are currently compiled with SassC, which with govuk-frontend v5 that includes source maps, fails to compile styles correctly - only including source maps. This could potentially be resolved by replacing SassC with a Dart Sass conduit for Ruby allowing us to keep the current asset
pipeline as-is.

Compile Javascript

govuk-frontend v5 dropped support for legacy browsers, so the preference when using it is to included as a type=module
That requires a new entry point in the core layout and a way to compile it / include it in addition to the current application.js as we do not want to make everyone change the way they write their own javascript in downstream projects.

Current flow is that downstream project has an application.js which requires any of the own local files, as well as govuk_tech_docs.js which comes from this gem. In turn, govuk_tech_docs.js requires govuk-frontend, jquery, etc.
So when, downstream run bundle exec middleman build everything gets compiled into application.js.

For govuk-frontend v5 we want to compile two files:

  • application.js as is now
  • govuk_frontend.js with govuk-frontend only

To try and achieve that in a way where a downstream project doesn't have to make any additions we explored

a) creating a govuk_frontend.js entry point in the lib: 39502c0
b) using Gulp with Middleman external pipeline: 26f13f0

WIth a) the problem found was that the generated govuk-frontend.js was served with middleman serve but not present in the build folder when middleman build was run.

With b) the problem is that any script to run say gulp with the gulpfile, would need to be present in the downstream projects package.json. We could not find a way to execute the script that lies in this gem. There is a hacky way to extract the path of the gulpfile from the the gem, but it doesn't feel right.

Using Middleman external pipeline feels like the way forward, but we'd need to find a way for a downstream project to trigger gem build steps on serve and build without needing to any additional installation steps.

Should above not be achievable, I believe the only other way to achieve separate entry points is:

  • us create say a govuk_frontend_all.js entry point in the lib that requires govuk-frontend
  • reference that file in the core.rb layout
  • write guidance for downstream to create a govuk_frontend.js file and require our govuk_frontend_all.js

Then their build should create all the necessary files. It's a similar flow to how application.js is constructed.

@huwd
Copy link
Member

huwd commented Sep 11, 2024

Bit I want to really exhaust is "are we really sure we can't do this with sprockets".

Things We've discovered:

  • as @kr8n3r says v4 of middleman switched to an expectation of an external asset pipeline
  • not sure that's ideal for this gem, as really we want to bundle some asset pipeline tools, so we can ship a "batteries included" govuk-frontend implementation and allow folks to write their own JS in the downstream docs site
  • this gem currently uses middleman-sprockets, which returns an internal pipeline into middleman (so something that can get shipped with the gem. This project is largely unmaintained 😭
  • One particular thing I need to 👀 is that this is locked to sprockets v3, whereas most up to date version is now v4 and comes with a list of features that may interest us

Bits left for me to work out:

  • with a sprockets v3 implementation / middleman-sprockets can we provide it with config to enable @kr8n3r's plan of separate endpoints above, I just need time to dig into the sprockets interface more
  • failing that... is there anything in the upgrade to v4 that helps there?

I'd really like to exhaust this line of thought as it's the least disruptive to the current delivery model of the tech-doc-gem, and keeps batteries included as much as possible without asking tech-doc folk to figure out how to run an external asset pipeline (which I think raises the congitive load for folks to use this)

@huwd
Copy link
Member

huwd commented Sep 11, 2024

@kr8n3r I think i've found something in sprockets that might help
Adding this line:
38fa85d

Creates a new file under assets/govuk/govuk-frontend.min.js which I think I should be able to reference as a separate entry point, no?

I testing that it does actually bring something across by creating a bloop.js that does console.log inside the node modules folder, adding that and I can now see it in by build directory downstream... so perhaps there's a lead there?

@kevindew
Copy link
Member

Rather than having to tie everything together in gem build and serve steps, I'd suggest going the rails route and having a bin/dev executable which can manage multiple processes. Then you can have dart sass process there and even a JS build too if needs be.

You can put the executable in an exe directory to have it bundled as part of the gem and could maybe explore setting the executable up as a binstub that installers of the gem use.

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 16, 2024

@huwd this produces an error during the build (CJS) as i don't think this pipeline handles es6 (esm).
As this is a minified ready to go file, I'll look into copying it directly like we do with other assets

error is because
minify_js only seemn works with ES5. for es6 ruby-terser looks to be a better option. if only we can use that with middleman-sprockets

so if we can switch minifiers, then we can provide a file where downstream can include it with a one-line in a new file
see f5e863c

Screenshot 2024-09-17 at 09 58 01 Screenshot 2024-09-17 at 09 48 07 Screenshot 2024-09-17 at 09 46 57

marcotranchino added a commit that referenced this issue Sep 17, 2024
This is needed because of the need to create two different JS rollups and
because the default compressor fails. See link below.

#352 (comment)
marcotranchino added a commit that referenced this issue Sep 17, 2024
This is needed because of the need to create two different JS rollups and
because the default compressor fails. See link below.

#352 (comment)
marcotranchino added a commit that referenced this issue Sep 17, 2024
This is needed because of the need to create two different JS rollups and
because the default compressor fails. See link below.

#352 (comment)
@huwd
Copy link
Member

huwd commented Sep 17, 2024

@kr8n3r what do we think about @kevindew's line above?

I hadn't considered using /bin/dev in that way... could be a way to get us shifting to the "external pipeline" model, dropping a bunch of old dependencies and bringing in modern / quick ones...

... but if I understand @kevindew's comment right still having this as something that ships with the gem, and so keeps the feature set here as "batteries included", rather than pushing the burden down for every docsite to manage itself.

@kr8n3r
Copy link
Contributor Author

kr8n3r commented Sep 17, 2024

not my area of expertise so cannot comment.
working draft of the current pipeline with v5 #363

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

No branches or pull requests

4 participants