Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 171 Remove JavaScript support for legacy browsers #171
RFC 171 Remove JavaScript support for legacy browsers #171
Changes from 2 commits
55ef11a
0f0d95f
9f4ca36
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, a couple of other things for us to consider when it comes to implementation:
.govuk-frontend-supported
for ES modules support govuk-frontend#3801type=module
attribute is added to the script tag, the script will automatically be deferred, for example, the scripts below run without waiting for any other events to fire, in this case the scripts just log the application name to the console.Not using type=module
Console output
Using type=module
Console output
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, makes sense to follow this approach to browser support. I imagine we may want to document our approach to browser support as well, making reference to the browser support recommendations from the design system and adding any further exceptions we have made where required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One additional thing to be aware of here is that scripts loaded with
<script type=module ...
implicitly have strict mode enabled.I think we're inconsistent in our use of strict mode currently - some modules set it in function scope, but many others don't.
That means this RFC also effectively rolls out strict mode everywhere, which is a good thing, but we'll have to consider whether there might be any risks of breakages while transitioning to strict mode.
We might want to consider whether to roll out strict mode module by module first, or whether to just check everything and do it all at once by setting
type=module
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
... I nerd sniped myself with this a bit... here's a draft PR which introduces linting for 'use strict' in the components gem:
https://github.com/alphagov/govuk_publishing_components/pull/4020/files
If you look at the lint results, there are a lot of places where we don't set it.
On the other hand, I'm pretty confident it's a moot point, because the linter would probably complain if we did anything that's not allowed in strict mode. So I think I'd be okay with just going for it and switching to type=module and getting strict mode everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, I think this also makes it difficult to communicate what users can expect to function as before by having partial/mixed support for IE11 (Grade X Browsers)