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

Enable UglifyJS compatibility workarounds #3137

Merged
merged 2 commits into from
Jan 16, 2023
Merged

Conversation

colinrotherham
Copy link
Contributor

@colinrotherham colinrotherham commented Dec 23, 2022

This PR lowers the threshold for function calls being "inlined" in UglifyJS to fix:

Running our minified bundle in IE8 sees the Event polyfill fail to patch Element.prototype.addEventListener due to polyfill code being "inlined" with unexpected side effects.

Various unpublished inline improvements been made since the last uglify-js@3.17.4 release:

These might fix it, but for now we can disable inlining functions with variables and statements

Before

compress: {
  inline: true // inline functions with arguments, variables and statements
}

After

compress: {
  inline: 2 // inline functions with arguments
}

Also included are various flags to opt out of optimisations that can break older browsers

module: false,
v8: true,
webkit: true

UglifyJS compress options

https://github.com/mishoo/UglifyJS#compress-options

inline (default: true) — inline calls to function with simple/return statement:

false — same as 0
0 — disabled inlining
1 — inline simple functions
2 — inline functions with arguments
3 — inline functions with arguments and variables
4 — inline functions with arguments, variables and statements
true — same as 4

@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 12:35 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 14:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 14:25 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 14:29 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 14:34 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 14:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:11 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:14 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:21 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:23 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:31 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:51 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 15:58 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 16:06 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 16:08 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 16:13 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 16:52 Inactive
@govuk-design-system-ci govuk-design-system-ci temporarily deployed to govuk-frontend-pr-3137 December 23, 2022 17:05 Inactive
@colinrotherham colinrotherham marked this pull request as ready for review December 23, 2022 17:20
@colinrotherham colinrotherham changed the title Enable JavaScript minifier compatibility options Enable JavaScript minifier compatibility workarounds Dec 23, 2022
@colinrotherham
Copy link
Contributor Author

colinrotherham commented Dec 23, 2022

Whilst it turned out not to be related, I've also added a missing Event polyfill to Accordion

Moved to:

@colinrotherham colinrotherham force-pushed the minify-uglifyjs-config branch from 935b242 to cf79eeb Compare January 9, 2023 13:42
@colinrotherham colinrotherham changed the base branch from main to accordion-event-polyfill January 9, 2023 13:42
@colinrotherham colinrotherham force-pushed the accordion-event-polyfill branch from 4f1d80e to e937fcb Compare January 9, 2023 13:51
@colinrotherham colinrotherham force-pushed the minify-uglifyjs-config branch from cf79eeb to 0cbd398 Compare January 9, 2023 13:52
Base automatically changed from accordion-event-polyfill to main January 9, 2023 15:49
@36degrees 36degrees self-requested a review January 10, 2023 14:17
@colinrotherham colinrotherham changed the title Enable JavaScript minifier compatibility workarounds Enable UglifyJS compatibility workarounds Jan 10, 2023
We’re seeing the `addEventListener` polyfill not applied in IE8 when UglifyJS `inline` optimisations above level 2 (no variables or statements) are added

https://github.com/mishoo/UglifyJS#compress-options
@colinrotherham
Copy link
Contributor Author

@36degrees I'm going to merge this in as initial testing looks good

Won't focus on it as we're nearly set to replace it with Terser once testing is complete:

@colinrotherham colinrotherham merged commit c5fc0e3 into main Jan 16, 2023
@colinrotherham colinrotherham deleted the minify-uglifyjs-config branch January 16, 2023 16:05
@colinrotherham colinrotherham added the 🐛 bug Something isn't working the way it should (including incorrect wording in documentation) label Jan 17, 2023
@36degrees 36degrees linked an issue Jan 26, 2023 that may be closed by this pull request
@owenatgov owenatgov mentioned this pull request Jan 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐛 bug Something isn't working the way it should (including incorrect wording in documentation) javascript tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Minified JS in dist errors in IE8
3 participants