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

Add browserslist 4 support. #8509

Merged
merged 5 commits into from Aug 24, 2018
Merged

Conversation

@yavorsky
Copy link
Member

yavorsky commented Aug 22, 2018

Q                       A
Fixed Issues? Fixes #8228
Patch: Bug Fix?
Major: Breaking Change? Yes
Minor: New Feature?
Tests Added + Pass? Yes
Documentation PR Link
Any Dependency Changes? browserslsit > 4
License MIT

After discussion with @ai and @existentialism and update browserslist to 4 we had an idea to possibly have targets as a query.
For now, browserslist 4 supports node. So in most cases, we don't need to use a plain object to specify targets directly. It's mostly confusing to have 2 sources of truth and merge this targets. For many people, it works like magic under the hood and it's not everytime clear why chrome: 55, browsers: 'chrome 40' will select chrome 55.
With browserslist 4 we just can specify every target we need using the query: targets: "node 8.1, last 2 chrome versions, maintained node versions".
Most of the people are using targets as a plain object just to support node version. This PR contains node current and current node versions which are being replaced to node ${currentNode} query.

So what was changed?

  1. updated browserslist to 4 and added node.js mappings.
  2. Allow targets to be as a query for browserslsit. (It's supposed that people won't use the object version in future and someday we could remove this functionality. Should be less confusing for users).
  3. Don't search for browserslistconfig if targets field was specified. (Since browserslist support node) (breaking change).
  4. support current node and node current queries, which is replaced with node ${CURRENT_VERSION}.

Would be good to merge it before babel 7, since it contains small breaking change (point 3).

cc @ai @existentialism

@yavorsky yavorsky requested a review from existentialism Aug 22, 2018
@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 22, 2018

Yeap, I highly recommend to include this PR to 7.0. People complained about the current behavior (merging targets and browserslist). And we will not be able to do it in next minor 7.x releases.

@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 22, 2018

Did you add current node to targets queries as the alias to last 1 node version? I can add this query to next Browserslist 4.1 release if Babel will use it.

@nicolo-ribaudo

This comment has been minimized.

Copy link
Member

nicolo-ribaudo commented Aug 22, 2018

In case we don't merge this before Friday, would it be possible to remove (or workaround) the breaking change?

@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 22, 2018

@nicolo-ribaudo it is possible, but will be bad for users. Merging behavior is what is expected (and how other tools do it).

@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 22, 2018

I added current node query support to Browserslist master browserslist/browserslist@40b4816

Let’s remove node current since aliases are not common in Browserslist and we try to keep queries more English-ish.

Can release it just after you will approve the query.

@yavorsky

This comment has been minimized.

Copy link
Member Author

yavorsky commented Aug 22, 2018

Sounds awesome @ai

"targets": {
"node": "4.0.0"
},
"targets": "node 4.0.0",

This comment has been minimized.

Copy link
@existentialism

existentialism Aug 22, 2018

Member

Instead of modifying existing tests, should we just add new ones to make sure both syntaxes work (for now)?

@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 23, 2018

Browserlist 4.1 was released with current node query support for Babel

@chicoxyzzy

This comment has been minimized.

Copy link
Member

chicoxyzzy commented Aug 23, 2018

Just my 2 cents: it will be breaking change for babel-preset-env, not Babel if we'll land it after Babel 7

@TrySound

This comment has been minimized.

Copy link
Contributor

TrySound commented Aug 23, 2018

@chicoxyzzy babel preset env version is synchronised with all packages. So it will be a breaking change for all stuff.

@chicoxyzzy

This comment has been minimized.

Copy link
Member

chicoxyzzy commented Aug 23, 2018

Actually it’s no so important to keep preset version in sync with Babel. More then that, this causes some issues. Also for example babel-loader and some other packages are not in sync with Babel version

@TrySound

This comment has been minimized.

Copy link
Contributor

TrySound commented Aug 23, 2018

All packages in babel monorepo has the same version. babel-loader is part of it.
https://github.com/babel/babel/tree/master/packages

yavorsky and others added 4 commits Aug 22, 2018
@existentialism existentialism force-pushed the browserslist-4-targets-as-query branch from c33997f to 0ebb6eb Aug 23, 2018
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Aug 23, 2018

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/8893/

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Aug 23, 2018

@ai @TrySound bumped to 4.1!

@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 23, 2018

Should we remove ignoreUnknownVersions: true if we are using 4.1? (It was added to fix current nide hack for 4.0)

@existentialism

This comment has been minimized.

Copy link
Member

existentialism commented Aug 23, 2018

@ai good point, will remove

@hzoo
hzoo approved these changes Aug 23, 2018
Copy link
Member

hzoo left a comment

Ok looks good, I think the breaking change makes sense (targets shouldn't be related to browserslist)?

@hzoo hzoo added this to the Babel 7 milestone Aug 24, 2018
@existentialism existentialism merged commit 4249dbc into master Aug 24, 2018
5 checks passed
5 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 80.55% (target 80%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@ai

This comment has been minimized.

Copy link
Contributor

ai commented Aug 24, 2018

Do we need something else to merge it?

Or just time for final decision

@loganfsmyth loganfsmyth deleted the browserslist-4-targets-as-query branch Aug 24, 2018
jaydenseric added a commit to jaydenseric/graphql-react that referenced this pull request Nov 1, 2018
Fixes Babel not reading from the package `browserslist` field due to a sneaky @babel/preset-env breaking change, see babel/babel#8509.
StoyanSimeonov added a commit to StoyanSimeonov/React-GraphQL that referenced this pull request Aug 8, 2019
Fixes Babel not reading from the package `browserslist` field due to a sneaky @babel/preset-env breaking change, see babel/babel#8509.
RyanWooldridge added a commit to RyanWooldridge/React-graphql that referenced this pull request Aug 21, 2019
Fixes Babel not reading from the package `browserslist` field due to a sneaky @babel/preset-env breaking change, see babel/babel#8509.
@lock lock bot added the outdated label Oct 4, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 4, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.