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

Make target browsers configurable #892

Closed
gaearon opened this issue Oct 11, 2016 · 46 comments
Closed

Make target browsers configurable #892

gaearon opened this issue Oct 11, 2016 · 46 comments
Milestone

Comments

@gaearon
Copy link
Contributor

gaearon commented Oct 11, 2016

Imagine package.json like this:

  "browsers": {
    "development": "last 1 version",
    "production": "last 2 versions, > 5%"
  }

The format is the same as browserslist.

Then, we would use this information both for Babel and Autoprefixer.

Autoprefixer already supports this format of browser configuration.

We could teach babel-preset-env to honor it too, or we could write a helper that translates browserslist to its target.

This would solve #435 with some optional configuration.

It would make rebuilds faster and also make output less mangled without introducing sourcemaps, helping work around #139.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 11, 2016

cc @hzoo

@hzoo
Copy link

hzoo commented Oct 11, 2016

👍 and FYI we have babel/babel-preset-env#19 in progress as an option already so that's a start 😄

@gaearon
Copy link
Contributor Author

gaearon commented Oct 11, 2016

Omg that's so cool. Wow. I'm excited.

@hzoo
Copy link

hzoo commented Oct 11, 2016

I think I got the data layer correct (mostly) so it's just on all of us to make sure that the data in https://kangax.github.io/compat-table/es6/ is correct (please check + make PRs to change this) - example I updated the table for node in compat-table/compat-table#932

Good if we get a lot of testing/eyes through usage.

There might be some issues with not compiling some features while compilng others but we'll just have to find those and fix them

@AlicanC
Copy link

AlicanC commented Oct 11, 2016

This is a step backwards for SSR and universal JS in general. Node is a very valid target for React applications and this proposal completely ignores that fact.

You (CRA and Babel) should strip the query parser from browserslist and query kangax instead of caniuse.

(Now I have to go and read the Yarn announcement.)

@gaearon
Copy link
Contributor Author

gaearon commented Oct 11, 2016

Sorry, I don't quite understand your comment.

Node is a very valid target for React applications and this proposal completely ignores that fact.

This is "Create React App" repo. The project explicitly does not support server rendering, nor does it intend to. Why is this a "step back" when server rendering is not supported by it?

If CRA supported Node, we'd detect Node environment exactly the same way. In fact we already do for tests: #878. So I still don't see why this would pose a problem even for people who eject and want to add server rendering.

The "browsers" field would not somehow make this code executable only by the browsers 😉 . It just lets us infer target environments as a moving target instead of assuming ES5 as a base. And tools like -env preset are just as relevant for Node as they are for browsers. Luckily we don't need a separate field for Node because we can just check process.version as we do in #878.

Could you please explain more?

@AlicanC
Copy link

AlicanC commented Oct 11, 2016

Oh, I'm sorry. I mistakenly thought CRA's view of SSR was changed from "no intent" to "some intent". Don't mind my comment then.

@hzoo
Copy link

hzoo commented Oct 13, 2016

Merged babel/babel-preset-env#19 and release 0.0.6 with "browsers" in the config

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Right now Browserslist already have a browserslist config.

It has no development/production versions and doesn’t support section in package.json.

We could add both features if it is really necessary :).

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

@ai The separation between dev/prod is important to us but I'd be happy to just keep it as part of CRA itself. If you'd like, you could adopt it, but then you'd have to figure out a conventional way to determine which config should be chosen which is tricky. In case of CRA we know this unambiguously because we have separate commands for running app in dev and prod. Reading NODE_ENV might be too error-prone because people often forget to set it.

@ai
Copy link
Contributor

ai commented Oct 13, 2016

It is hard to make it CRA-only but use in other user tools. Like when user add Stylelint, it will not share common config from package.json.

Why dev/prod is important? Maybe we could find better solution?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

It is hard to make it CRA-only but use in other user tools.

Isn't it possible to pass the desired list to browserlist function, and have it override any configs? Then we'd just read our package.json and pass that to it from CRA.

Why dev/prod is important? Maybe we could find better solution?

This would let people transpile less in development because they can specify they develop on modern browsers. Both Babel and Autoprefixer would do less work and produce cleaner output, but for production they'd do the more conservative transformations.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

OK, I think I get it now.

Like when user add Stylelint, it will not share common config from package.json.

Fair enough, but then this is the case now as well, where browserslist is not respected at all. At least it's a step in the right direction, wouldn't it be?

Maybe browserslist could support

{
  env1: list1,
  env2: list2
}

notation and accept environment as an argument.

If it is not specified, all rules are summed up (env1 + env2) to be more conservative. If specified, only the right environment is picked.

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Browserslist format now is very easy. For compatibility we could suggests:

[production]
last 2 version
# because of X client
ie 9

[development]
node >= 4

So you suggests to have production and development to keep development build cleaner? But is it a good practice. In my experience development should be as much as closer to production. Only real slow tasks (like minification) should be moved to production.

For example, nobody asked Autoprefixer for development browsers list. Because extra prefixes could potentially change something.

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

Just production/development distinction would work fine for us.
The only feature we'd need in addition to this is that it can be specified package.json.

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Sure, I will add package.json support under browserslist key.

You didn't answered why it is a good practice 😉. I think it is very dangerous to have different environment.

I could add it. But I want some rational reasons. I afraid that different environments will saves us a few milliseconds, but will increase production errors, which we could not debug in development.

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Issue about package.json: browserslist/browserslist#74

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Issue about node.js versions support: browserslist/browserslist#75

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

I don't have numbers before and after so I'd be thrilled if somebody could compile them, but I expect wins from disabling most ES2015 transforms to be substantial. The class transform output is also extremely noisy, and annoying to React users.

We use Babel in non-loose mode which is pretty conservative. Babel 6 has also been out for a while, and the vast majority of bugs has been squashed since.

My opinion is that this is safe enough to go as an opt-in option.

@ai
Copy link
Contributor

ai commented Oct 13, 2016

@gaearon let’s ask Babel developers: @hzoo

  1. do any Babel users use different plugins for dev/prod?
  2. do users ask about it often?
  3. it is 99,99-100% safe to have different Babel plugins in dev/prod?

@hzoo
Copy link

hzoo commented Oct 13, 2016

do any Babel users use different plugins for dev/prod?

This should definetely be true because we have the env option for babel configs. Usually a test, dev, ci, or prod env. For example you could use plugins that deal with introspection (code coverage), or http://babeljs.io/docs/plugins/transform-react-jsx-source/, or optimizations only in prod.

Given evergreen browsers support most of ES6 some users have been thinking about only running the import/export transform (me as well). This is why I wanted to make the env preset (it was suggested multiple times very early on babel/babel#272) but the maintenance burden was too high).

There are a few cases where turning off transforms will cause bugs because maybe less people are doing that but we should be able to fix those as more people decide to do so. The only big issue is with https://babeljs.io/docs/plugins/transform-object-rest-spread/ which requires the parameters/destructuring transform in certain cases because it isn't isolated enough.

it is 99,99-100% safe to have different Babel plugins in dev/prod?

Not 100% since if there is a bug in the plugin that isn't in dev then you won't see it. But I guess that's why we have a stage env usually?

@gaearon
Copy link
Contributor Author

gaearon commented Oct 13, 2016

The only big issue is with https://babeljs.io/docs/plugins/transform-object-rest-spread/ which requires the parameters/destructuring transform in certain cases because it isn't isolated enough.

Does this affect us after #878? Should we explicitly whitelist destructuring?

@ai
Copy link
Contributor

ai commented Oct 13, 2016

OK, if Babel users already use env, we should not ignore it (I still think it is dangerous practice :D)

Issue about environments: browserslist/browserslist#76

@ai
Copy link
Contributor

ai commented Oct 13, 2016

Could you check all that issues? Did I forget something?

@AlicanC
Copy link

AlicanC commented Oct 13, 2016

do any Babel users use different plugins for dev/prod?

Babili

@hzoo
Copy link

hzoo commented Oct 13, 2016

It will error out when you use

// ok
var a = { ...b } // most common use case
// error
function foo({ x, y, ...z }) {} // spread in parameters (need parameters-transform)
const { c, ...a } = a; // rest in destructuring

Sounds like it's my next priority 😛

@oallouch
Copy link

Any news on this ? Chrome Canary is now in version 56 and async/await is here (stable) since v55.

@gaearon
Copy link
Contributor Author

gaearon commented Feb 24, 2017

We currently don't use env yet for browser builds, only for Node.
So I'll keep it open for now.
Thanks!

cr101 added a commit to cr101/create-react-app that referenced this issue May 25, 2017
…he browserslist key in package.json which allows developers to set browsers list in queries like "Chrome >= 35", "Firefox >= 38" for both production and development. Autoprefixer which relies on Browserslist will find its config automatically. This PR fixes facebook#2248 and facebook#892
@mohammedzamakhan
Copy link

Any updates on this @gaearon, the application I am working on, we only support Chrome, I hope the bundle size will decrease a lot if we have this working

@foaly-nr1
Copy link

@mohammedzamakhan it's part of the 100.0.0 milestone which is currently at 71%: https://github.com/facebookincubator/create-react-app/milestone/8

@stereobooster
Copy link
Contributor

What is the final format will be?

Dan originally proposed:

"browsers": {
  "development": "last 1 version",
  "production": "last 2 versions, > 5%"
}

Browserslist implementation:

"browserslist": {
  "development": ["last 1 browser"],
  "production": ["last 4 browser"]
}

@ai
Copy link
Contributor

ai commented Dec 26, 2017

Browserslist already implement it. Should we close this issue?

Also I can send PR with moving Autoprefixer's browsers from webpack config to package.json template, since we don't recommend browser option.

@Timer
Copy link
Contributor

Timer commented Jan 2, 2018

Browserslist already implement it. Should we close this issue?

We need to change how some things work internally first so that it's applied to babel & css.

@ai
Copy link
Contributor

ai commented Jan 2, 2018

@Timer I sent a PR for it (if I understood it correctly) #3644

@Timer
Copy link
Contributor

Timer commented Jan 13, 2018

Autoprefixer: #3644
Babel: #3770

@Timer Timer closed this as completed Jan 13, 2018
@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

10 participants