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(gatsby): Multi-environment browserslist configs #35081

Conversation

chrissantamaria
Copy link
Contributor

Description

This PR fixes the logic in getBrowsersList to properly handle multi-environment Browserslist configs.

More info available in #29943, though the root issue is the current logic always choosing config.defaults - with a multi-environment config, this may return [] which (improperly) passes through the current logic check. This can have some pretty undesirable effects on parts of the build, including it currently breaking the hasES6ModuleSupport helper and causing a build failure (see the reproduction repo).

Fortunately, the fix is pretty simple: browserslist exposes a loadConfig function which has all of the necessary logic for picking the right config, including falling back to defaults for the majority of cases. I haven't found a case where this breaks existing behavior, though please double check me on this!

As an added bonus, loadConfig also respects the BROWSERSLIST_ENV and BROWSERSLIST_CONFIG environment variables which should make customizing build behavior a bit simpler.

Related Issues

Fixes #29943

@gatsbot gatsbot bot added the status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer label Mar 9, 2022
@LekoArts LekoArts added topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine) and removed status: triage needed Issue or pull request that need to be triaged and assigned to a reviewer labels Mar 10, 2022
@LekoArts LekoArts changed the title Fix build error with multi-environment Browserslist configs fix(gatsby): Multi-environment browserslist configs Mar 10, 2022
@wardpeet wardpeet self-assigned this Mar 28, 2022
@chrissantamaria chrissantamaria force-pushed the fix-browserslist-multi-environment-detection branch from d5b8513 to 2c0f1f0 Compare January 11, 2023 18:06
@chrissantamaria
Copy link
Contributor Author

@wardpeet @LekoArts sorry for the ping - just wanted to check on the status of this PR. I've confirmed this is still an issue on the latest Gatsby version (see updated reproduction repo) and that my PR still fixes the issue. I've rebased the PR with the latest master.

It's been nearly 2 years since the original issue was opened 🍰 so I'd really like to push this forward. Happy to make any necessary changes - thanks!

Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Really sorry for the long wait! LGTM 👍

@LekoArts LekoArts merged commit 5e7e8fa into gatsbyjs:master Jan 12, 2023
@chrissantamaria chrissantamaria deleted the fix-browserslist-multi-environment-detection branch January 12, 2023 15:26
@chrissantamaria
Copy link
Contributor Author

chrissantamaria commented Jan 12, 2023

Thanks for merging! 🎉

As a heads up, this PR should fix the underlying issue behind #29954 if you want to revert back to a Webpack target of browserslist - looks like this might result in more modern runtime code generation rather than always falling back to a target of es5. Tried it out locally and seems to (slightly) reduce overall bundle size, though I haven't benchmarked any performance improvements.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: core Relates to Gatsby's core (e.g. page loading, reporter, state machine)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incomplete Browserslist support in build
3 participants