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

Specify browserslist uses 'defaults' #363

Merged
merged 1 commit into from
Aug 30, 2021
Merged

Conversation

jblz
Copy link
Contributor

@jblz jblz commented Aug 30, 2021

Description

Specify that the browserslist should use the defaults setting when building bundles.

Motivation and Context

I noticed that unrelated script files were being altered during build. This is suboptimal since it bundles build changes into unrelated PRs making it more difficult to pin down where issues arise (if they do).

What Happened?

It looks like the @wordpress/scripts package has begun to fine tune the build targets to the specific browser support level in the handbook:

Last 1 Android versions.
Last 1 ChromeAndroid versions.
Last 2 Chrome versions.
Last 2 Firefox versions.
Last 2 Safari versions.
Last 2 iOS versions.
Last 2 Edge versions.
Last 2 Opera versions.
Browsers with > 1% usage based on can I use browser usage table

This is manged in code in the @wordpress/browserslist-config package here

Meanwhile, the default set of targets in the browserslist package is larger and includes older user agents such that the resultant built files exclude newer functionality (e.g. variables are declared via the var keyword (instead of const or let):

> 0.5%
last 2 versions
Firefox ESR, not dead

This changed at some point after #354 landed since that PR did not result in any change to the built files. I confirmed that I was able to check out the hash prior to updating the @wordpress/scripts package (69bb168) and install deps / build our scripts without any changes to any versioned files.

Alternatively

Perhaps we do want to follow the lead of the WordPress tooling and adopt a more modern set of targets. If that's what we decide, we can close this PR and PR the build as it is.

How Has This Been Tested?

Confirm the issue

  • Check out origin/develop
  • npm i
  • npm run build
  • git status / git diff
    • Displays that built files have changed

Confirm the fix

  • Check out this branch
  • npm i
  • npm run build
  • git status / git diff
    • Displays that no built files have changed

Screenshots (if appropriate):

Types of changes

Dev tooling

@jblz jblz requested a review from a team August 30, 2021 22:22
@jblz jblz self-assigned this Aug 30, 2021
Copy link
Contributor

@GaryJones GaryJones left a comment

Choose a reason for hiding this comment

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

I'd love to reduce noise on PRs, so +1 to this.

@jblz
Copy link
Contributor Author

jblz commented Aug 30, 2021

Thanks for looking @GaryJones -- if we decide we do want to tighten our browserslist, we can do it in the future. I'm going to go ahead and merge this for now, though. As I have some changes in the works.

@jblz jblz merged commit 3b7031b into develop Aug 30, 2021
@jblz jblz deleted the browsers-list-defaults branch August 30, 2021 22:40
@pauarge pauarge mentioned this pull request Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants