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

Swap JavaScript minifier from UglifyJS to terser #3013

Merged
merged 2 commits into from
Jan 17, 2023

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Nov 15, 2022

Enable minification of ES6+ features

Swaps UglifyJS with terser (forked version) as explained on the terser README.md

uglify-es is no longer maintained and uglify-js does not support ES6+.

terser is a fork of uglify-es that mostly retains API and CLI compatibility with uglify-es and uglify-js@3.

Related:

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 15, 2022 17:07 Inactive
@colinrotherham colinrotherham added this to the v5.0 milestone Nov 15, 2022
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 17, 2022 16:45 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 18, 2022 12:27 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 22, 2022 10:43 Inactive
@colinrotherham colinrotherham changed the base branch from main to gulp-plumber-rollup November 28, 2022 14:49
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 28, 2022 14:50 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 28, 2022 17:19 Inactive
Base automatically changed from gulp-plumber-rollup to main November 28, 2022 17:26
@romaricpascal

This comment was marked as outdated.

@romaricpascal
Copy link
Member

Besides the sourcemap being missing, looks like Uglify was quite "aggressive" in its minification with things like:

  • inlining function body that were used once, like normaliseString
  • reusing variables, like in Tabs.prototype.getHref

Not really a concern, more of an observation and something we may want to look into in the future to see how far we can minify things again tweaking terser's options.

I think I'd me more confident for these tweaks to happen after we run our review app against the minified files, as you proposed. Which actually would give us safety to merge this PR as well, so maybe we could look into making the review app use a minified file and then merge this one?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 29, 2022 14:29 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 29, 2022 15:03 Inactive
@36degrees
Copy link
Contributor

What are the things that we're worried about with this change?

As this only affects dist, I can't immediately see any reason this needs to wait until v5.0 as long as browser support doesn't change and the file size isn't massively increased.

@colinrotherham colinrotherham changed the base branch from main to minify-all-by-default November 29, 2022 15:07
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 November 29, 2022 15:07 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 16, 2023 19:49 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 16, 2023 19:55 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 16, 2023 19:56 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 16, 2023 19:57 Inactive
@colinrotherham
Copy link
Contributor Author

@36degrees Added a CHANGELOG entry under Fixes

Fixes

We've fixed errors in IE8 caused by updates to our JavaScript minifier UglifyJS. The issue was limited to the release-v4.4.1.zip and release-v4.4.0.zip assets on GitHub releases and prevented some polyfills from running:

Too much info?

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 16, 2023 21:36 Inactive
@36degrees
Copy link
Contributor

LGTM, but worth checking with @claireashworth.

Might be worth using the term 'precompiled files' or 'precompiled JavaScript' as part of the description to match the terms we use in our install docs?

@colinrotherham
Copy link
Contributor Author

Thanks @36degrees I'll hold on for @claireashworth

I like 'precompiled JavaScript' but which bit would you swap out?

Copy link
Contributor

@claireashworth claireashworth left a comment

Choose a reason for hiding this comment

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

This looks fine to me. You could shuffle the text around to mention the polyfills not running sooner, but this para is short so not an essential change.

edit: I assume Ollie's suggestion would be here - We've fixed errors in IE8 caused by updates to our precompiled JavaScript...

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3013 January 17, 2023 10:08 Inactive
@colinrotherham
Copy link
Contributor Author

@claireashworth Fab, I've pushed again

@36degrees I've switched to JavaScript minifier UglifyJS "precompiled JavaScript" as all the necessary info on both UglifyJS and terser can be found in the PRs—that alright?

Fixes

We've fixed errors in IE8 caused by updates to our precompiled JavaScript. The issue prevented some polyfills from running but was limited to the release-v4.4.1.zip and release-v4.4.0.zip assets on GitHub releases:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file javascript tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minified JS in dist errors in IE8
5 participants