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

State of targeting browers #3774

Closed
Timer opened this issue Jan 13, 2018 · 5 comments
Closed

State of targeting browers #3774

Timer opened this issue Jan 13, 2018 · 5 comments
Milestone

Comments

@Timer
Copy link
Contributor

Timer commented Jan 13, 2018

I think we've all agreed targeting browsers would be an acceptable configuration. For 2.0, we tried giving this a shot -- here's the current state:

Parsing target browsers (#3644)

This change told our build pipeline to start looking for browserslist configuration.

When this was merged, the only real effect it had was CSS prefixing. It was not a complete implementation.
Our Babel configuration was still locked down to ES5.

Concerns

  1. Speed: what effect does this have on build time? Previously, we fed the configuration that now needs to be found (potentially found thousands of times depending on the build pipeline plugin efficiency)
  2. Configuration hell: We wanted to support browserslist in package.json, but we now support 5 means of configuration. I think we need to lock this down.
  3. What happens when nothing is specified? I'm not sure if we align with the browserslist default.

Minifying code ES6+

We know we want to support the latest syntax, so we merged #3618 -- this PR switched us from uglify-js to uglify-es under the hood, which supports new syntax.

Concerns

  1. This now lets node_modules with bad syntax leak into the build without being checked.
  2. Specify JavaScript version for Uglify #3743 tried to solve this but upon further digging, the option is used twice in uglify-es and does not actually limit syntax support in any way.

Outputting ES6+ code

Since #3644 didn't affect our compilation process, #3770 was merged.

#3770 removes our forced IE9/Uglify compatibility and instead lets @babel/preset-env use its new behavior of identifiying browser support via browserslist.

Concerns are shared with above:

  1. Speed -- what kind of performance hits are we going to see?
  2. Configuration hell (see above).
  3. node_module validity
  4. Defaults when nothing is specified?

Compiling node_modules

Yet to be solved.

Concerns

  1. Not all packages specify an engines field
@Timer
Copy link
Contributor Author

Timer commented Jan 13, 2018

Potential solutions

Solving speed, configuration hell, and defaults

What if pre-eject we loaded and identified the targeted browsers ourselves?
We could make a helper in react-dev-utils and forward detected browsers to our Babel preset and Autoprefixer.

This would fix any speed concerns and allow us to enforce a default other than what browserslist determines (we want our default to be ie 11).

Solving node_modules syntax validity

Three options, currently:

We could do nothing and let the ecosystem catch up -- this is dependent on compiling node_modules (via our engines idea).

We could also petition uglify-es actually limits language features based on ecma specification.

Alternatively, we could drop uglify-es and switch to babel-minify. babel-minify would allow us to use the syntax support enabled by preset-env/browserslist and ensure no code leaks through.
This means every module needs to be parsed by Babel -- we could fix speed concerns here with accurate caching.

@Timer Timer added this to the 2.0.0 milestone Jan 13, 2018
@gaearon
Copy link
Contributor

gaearon commented Jan 13, 2018

potentially found thousands of times depending on the build pipeline plugin efficiency

Can you clarify this? I’d assume it’s only queried once during startup. (Okay, twice: by autoprefixer and babel-preset-env.) Is that not the case?

we now support 5 means of configuration. I think we need to lock this down.

I think it’s not too bad if we add a post-build message to production build with the chosen browserlist when it differs from the one in package.json. It could include a link to browserslist docs.

Outputting ES6+ code [...] Speed -- what kind of performance hits are we going to see?

Do you only mean browserlist location? I would expect speed to be no worse since run the same number of transforms or less. (Speaking for the app code only.)

Not all packages specify an engines field

We could also go the other way and compile everything by default, skipping things that we know to be compatible. Maybe with caching it won’t be too bad?

@Timer
Copy link
Contributor Author

Timer commented Jan 13, 2018

Can you clarify this? I’d assume it’s only queried once during startup. (Okay, twice: by autoprefixer and babel-preset-env.) Is that not the case?

Fueled by this issue: webpack-contrib/eslint-loader#206. I just want to make sure preset-env and autoprefixer don't do something similar (I'm not saying they do, we should just check).

add a post-build message to production build with the chosen browserlist when it differs from the one in package.json

I'm OK with this.

Do you only mean browserlist location?

Yes, strictly browserslist location.

We could also go the other way and compile everything by default, skipping things that we know to be compatible. Maybe with caching it won’t be too bad?

Are we confident babel 7 will no longer break code? If so, this might be fine -- but I'd almost rather make compilation opt-in via engines rather than opt-out to we can force the ecosystem to move forward.

edit: then again, there's not really a good way to let users specify browser support -- just Node support. We need to talk more about this.

@Timer
Copy link
Contributor Author

Timer commented Jan 13, 2018

Merging into #3777, we hashed this out.

@Timer Timer closed this as completed Jan 13, 2018
@andriijas
Copy link
Contributor

@Timer thanks for all the efforts you are putting into browser targeting!

@lock lock bot locked and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants