Skip to content

exclude unnecessary plugins for target browsers #8030

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

heygrady
Copy link
Contributor

@heygrady heygrady commented Nov 26, 2019

Some of the advanced babel-preset-react-app customizations inadvertently re-enable plugins that @babel/preset-env disables (based on browserslist).

I ran into this issue while enabling modern builds for my application. I noticed that destructure and spread transforms were being applied even for very future-forward browser targets like last 1 chrome version.

This PR uses some of the internals of @babel/preset-env @babel/helper-compilation-targets and @babel/compat-data to know which plugins to apply and disables some customizations when they are not necessary given the browser targets.

@ianschmitz
Copy link
Contributor

Hey @heygrady, thanks for the hard work on this.

I'm thinking that maybe a cleaner approach is to remove the use of the plugins and allow preset-env to do the work. Previously when we added theses plugins I believe they weren't yet at stage 4, and therefore not transformed by preset-env by default. Now that they're standardized i feel we should use preset-env unless there's a good reason to opt for the plugin.

@babel/plugin-transform-destructuring is one we'd want to pay special attention to as we've added some optimizations for hooks here. That being said all modern browsers have long supported destructuring (chrome 49, safari 8, etc), so unless targeting ie11 or older our users wouldn't experience the deoptimization.

@ianschmitz ianschmitz added this to the 3.4 milestone Nov 26, 2019
@heygrady
Copy link
Contributor Author

heygrady commented Nov 26, 2019

Whatever works is fine by me.

The other plugins (like plugin-proposal-class-properties and plugin-proposal-numeric-separator) could be added to the data and we could check for those too in cases where people were targeting only browsers known to support those proposals.

Meaning, I thought it was interesting to use some of the mechanisms that preset-env uses to deepen our support for browserslist and allow plugins to be automatically managed that way.

I've just finished a project to support multiple builds for multiple targets using my fork of react-scripts. I found it useful to rely on those same mechanisms in the webpack config, for instance, for setting terser config options based on browserslist.

Regardless, it would be nice to have the bug in babel-preset-react-app fixed one way or another.

@heygrady
Copy link
Contributor Author

Pushed a commit where I removed @babel/plugin-proposal-object-rest-spread

Copy link
Contributor Author

@heygrady heygrady left a comment

Choose a reason for hiding this comment

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

I don't believe it's possible to side-step the need to know what plugins @babel/preset-env would apply and still support modern browsers.

  1. We need to update configuration to @babel/preset-react for the use of spread
  2. We need to disable regenerator in @babel/plugin-transform-runtime when it isn't needed
  3. We need to skip @babel/plugin-transform-destructuring when it isn't needed

Comment on lines +113 to +115
[isObjectRestSpreadProposalRequired
? 'useBuiltIns'
: 'useSpread']: true,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we need to use native spread whenever possible. This requires knowing if our targets require 'proposal-object-rest-spread'.

@@ -176,7 +182,7 @@ module.exports = function(api, opts, env) {
// explicitly resolving to match the provided helper functions.
// https://github.com/babel/babel/issues/10261
version: require('@babel/runtime/package.json').version,
regenerator: true,
regenerator: isRegeneratorTransformRequired,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we want to disable regenerator when it's not needed.

Comment on lines +134 to 139
// Historically there was a bug that made this plugin required.
// https://github.com/babel/babel/issues/7215
// This plugin is no longer required to make plugin-proposal-object-rest-spread work:
// https://github.com/babel/babel/pull/10275
isDestructuringTransformRequired && [
require('@babel/plugin-transform-destructuring').default,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here we need to undo this custom override when it's not needed.

@ianschmitz
Copy link
Contributor

I'll bring it up with the team and see what we think is the best approach.

@iansu iansu modified the milestones: 3.4, 3.5 Feb 14, 2020
@ianschmitz ianschmitz removed their assignment May 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants