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

fix(bundling): check for browserslist when setting terser ecma #17620 #17708

Merged

Conversation

Coly010
Copy link
Contributor

@Coly010 Coly010 commented Jun 21, 2023

Current Behavior

We do not check browserslist files in the withNx helper.

This can lead to outputs that are not compatible with the targetted browsers

Expected Behavior

We should let browserslist help define the ecma version for terser to use

Related Issue(s)

Fixes #17620

Notes

An improvement could be to find a method to determine the lowest ecma version based on all the browsers matching the browserslist queries

@Coly010 Coly010 requested a review from a team as a code owner June 21, 2023 14:48
@Coly010 Coly010 requested a review from mandarini June 21, 2023 14:48
@Coly010 Coly010 self-assigned this Jun 21, 2023
@vercel
Copy link

vercel bot commented Jun 21, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
nx-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 21, 2023 5:22pm

@nx-cloud
Copy link

nx-cloud bot commented Jun 21, 2023

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 2b1d53e. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@Coly010 Coly010 force-pushed the webpack/set-terser-ecma-to-browserslist-if-exists branch from bc22a61 to 2b1d53e Compare June 21, 2023 17:19
@Coly010 Coly010 enabled auto-merge (squash) June 21, 2023 17:26
@Coly010 Coly010 merged commit 6305e7f into nrwl:master Jun 21, 2023
@kevinbeal
Copy link

Thank you guys for doing this!

This may just be nitpicky stuff I'm bringing up, but the browser I discovered the issue on was a version of Safari, so while this is certainly an improvement, it wouldn't fix the issue for browserlists targeting older Safari but not IE11 (since the fix only checks for IE11 and otherwise picks es2020). Of course, I can just target IE11 and thus also cover old Safari, but I thought I would mention it anyway.

@Coly010 Coly010 deleted the webpack/set-terser-ecma-to-browserslist-if-exists branch June 22, 2023 09:33
@Coly010
Copy link
Contributor Author

Coly010 commented Jun 22, 2023

@kevinbeal Do you know what version of Safari it is? I can add it to an array of browsers to change to ES5.

There's no good way to get the ecmascript version that works across all the browsers found with browserlist unfortunately.

@kevinbeal
Copy link

@kevinbeal Do you know what version of Safari it is? I can add it to an array of browsers to change to ES5.

It was 13.0 on desktop and 13.3 on mobile (admittedly with small market shares), but Android 4.4.4 is affected too (with a slightly larger market share).
image

A quick look at the Terser source gives the impression that optional chaining and nullish coalescing are the only two compression checks they do beyond ecma2015 and both were introduced at the same time in each browser. There doesn't appear to be any benefit to es2016/17/18/etc checking.

There's no good way to get the ecmascript version that works across all the browsers found with browserlist unfortunately.

I noticed that too. I wonder why not. Seems like an obvious feature, but maybe that's just hindsight or my ignorance speaking 😅

@Coly010
Copy link
Contributor Author

Coly010 commented Jun 22, 2023

I noticed that too. I wonder why not. Seems like an obvious feature, but maybe that's just hindsight or my ignorance speaking 😅

@kevinbeal It's because some browsers introduced partial support for modern ES features and not total feature parity with each ES spec

Thanks for highlighting the browsers, I'll get the list of browsers updated!

@github-actions
Copy link

This pull request has already been merged/closed. If you experience issues related to these changes, please open a new issue referencing this pull request.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Terser plugin set to 2020, thus shortening using nullish coalescing operator despite browserslist
3 participants