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

Update review app to use new JavaScript loading strategy #3508

Closed
4 tasks done
Tracked by #2639
romaricpascal opened this issue Apr 14, 2023 · 8 comments · Fixed by #3769 or #3833
Closed
4 tasks done
Tracked by #2639

Update review app to use new JavaScript loading strategy #3508

romaricpascal opened this issue Apr 14, 2023 · 8 comments · Fixed by #3769 or #3833
Milestone

Comments

@romaricpascal
Copy link
Member

romaricpascal commented Apr 14, 2023

What

Update the snippet setting a class on the on the document's body to mark where GOV.UK Frontend is supported with a govuk-frontend-supported class, in complement of the js-enabled one marking where JavaScript is available.

That class will be set on the detection of noModule in HTMLScriptElement.prototype as it's the best way we've found to identify the browsers we support.

Why

We currently set a js-enabled class on the body to signpost that JavaScript is available in the browser and hook the styles of our components to it. It lets us style our components as early as possible with the assumption that they’ll be enhanced by JavaScript. This avoids a flicker of components showing their “no JavaScript styles” before JavaScript loads.

We’ll keep that to avoid breaking styles of services that may still want to run their own components on good old <script> tags like they do now.

We’ll complement that class with a new govuk-frontend-supported one that highlights that GOVUK Frontend can be initialised.

Who needs to work on this

Developers

Who needs to review this

Developers

Done when

Tasks

@colinrotherham
Copy link
Contributor

@colinrotherham
Copy link
Contributor

colinrotherham commented Jun 19, 2023

Do we want to continue using window globals for modules?

<script type="module" src="/javascripts/all.min.js"></script>
<script type="module">window.GOVUKFrontend.initAll()</script>

Aware that we shimmed the window.GOVUKFrontend global (only found in UMD bundles) for ES modules in:

Or can we decide to use modules as intended?

<script type="module">
  import { initAll } from '/javascripts/all.min.js'
  initAll()
</script>

We'd need to keep <script type="module" src="/javascripts/all.min.js"></script> for early discovery + download until browser support for <link rel="modulepreload"> support improves

@colinrotherham
Copy link
Contributor

Caught up with @romaricpascal yesterday and we're happy that window globals are no longer necessary in ES modules so we've switched to import rather than window.GOV.UKFrontend.* as mentioned above:

We'll continue to support UMD bundles (with window globals) in v5 but will deprecate them in future

@romaricpascal
Copy link
Member Author

We'll want to update the review app further for testing purposes, as the review app and its tests are in a bit of a wonky state. The app neither follows:

  1. an import version of frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#add-the-javascript-file-to-your-html, where we would serve node_modules/govuk-frontend/dist/govuk/all.mjs without bundling
  2. frontend.design-system.service.gov.uk/importing-css-assets-and-javascript/#import-javascript-using-a-bundler, where the bundled file would be the one initialising the components

So we're not quite testing that GOV.UK Frontend does what it's meant to in our tests, more that our review app bundle does what GOV.UK Frontend is meant to do.

I think 1. will be the option as our tests check what's available in the exports but we may want to test different things (eg. all.mjs providing exports but all.bundle.js setting the right window globals).

@romaricpascal romaricpascal reopened this Jun 22, 2023
@colinrotherham
Copy link
Contributor

colinrotherham commented Jun 23, 2023

@romaricpascal Yeah that's my plan, just need to replace all.mjs in 1) with all.bundle.mjs from #3726

We'd go back to it being unminified so probably good to add all.bundle.min.mjs too

Update: I've tried it out as part of #3726

  1. Including minified bundles *.bundle.min.mjs in e881f41
  2. Updating the review app to import from govuk-frontend without bundling in 7c80453

@colinrotherham
Copy link
Contributor

We'd need to use a bundled version to avoid a big network waterfall (as child modules are discovered and downloaded)

@36degrees
Copy link
Contributor

Is there anything left to do on this?

@colinrotherham
Copy link
Contributor

Is there anything left to do on this?

@36degrees Looks like we're all done 🎉

There's a tiny bit of <script> documentation that needs updating but it's not review app related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
3 participants