Skip to content
This repository has been archived by the owner on May 11, 2018. It is now read-only.

Adds browsers property to use browserslist's queries #19

Merged
merged 12 commits into from
Oct 13, 2016

Conversation

yavorsky
Copy link
Member

@yavorsky yavorsky commented Oct 7, 2016

This PR allows us to use browsers property with query like autoprefixer does.

For example, babelrc:

{
  presets: [
    ["env", {
      "targets": {
        "chrome": 52,
        "browsers": ["safari > 7"]
      },
      "loose": true
    }]
  ]
}

will include chrome 52 and safari 8, 9, 10.

ios_saf, ie11 and others unsupported browsers will be ignored.

@yavorsky yavorsky changed the title Adds browsers property to use browserslist Adds browsers property to use browserslist's queries Oct 7, 2016
@hzoo
Copy link
Member

hzoo commented Oct 7, 2016

awesome!

If you can make test for this as well with a few cases that would be great

@@ -52,6 +52,7 @@
"babel-plugin-transform-flow-strip-types": "^6.8.0",
"babel-preset-es2015": "^6.14.0",
"babel-register": "^6.14.0",
"browserslist": "^1.4.0",
Copy link
Member

Choose a reason for hiding this comment

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

This will have to be a dependency if we are using it in src

Copy link
Member

Choose a reason for hiding this comment

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

Although it would be better if it was used as a devDependency

@@ -62,7 +63,16 @@ export const isPluginRequired = (supportedEnvironments, plugin) => {
};

const getTargets = targetOpts => {
return targetOpts || {};
const mergedOpts = targetOpts || {};
const browserOpts = targetOpts['browsers'];
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to do any validation here? (like in loose/modules we verify if boolean etc)

Copy link
Member

Choose a reason for hiding this comment

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

Oh I see it's ignored right below

@yavorsky
Copy link
Member Author

yavorsky commented Oct 8, 2016

I think, browsers defined in targets' root must have higher priority than items we receiving in browsers query.
So,

{
  targets: {
    chrome: 50,
    safari: 10,
    browsers: ['last 2 chrome versions', 'last 2 safari versions'] // chrome 52, safari 9
  }
}

will output {chrome: 50, safari: 10}.

@hzoo
Copy link
Member

hzoo commented Oct 8, 2016

Yep I think so to, we should update the doc/comment to say that explicit targets will override what's in browsers

} else {
mergedOpts = targetOpts;
}
return mergedOpts;
Copy link
Member

@hzoo hzoo Oct 8, 2016

Choose a reason for hiding this comment

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

You could also do an early return here

const browserOpts = targetOpts.browsers;
if (isBrowsersQueryValid(browserOpts)) {
  const queryBrowsers = getLowestVersions(browserslist(browserOpts));
  return mergeBrowsers(queryBrowsers, targetOpts);
}
return targetOps;

@@ -39,6 +40,10 @@ export const MODULE_TRANSFORMATIONS = {
* @return {Boolean} Whether or not the transformation is required
*/
export const isPluginRequired = (supportedEnvironments, plugin) => {
if (supportedEnvironments.browsers) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this necessary?

The code that calls this does

 const targets = getTargets(opts.targets);
isPluginRequired(targets, pluginList[pluginName]));

Copy link
Member Author

Choose a reason for hiding this comment

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

@hzoo It's just for usage from a separate module. When options are not handled yet.

Copy link
Member Author

Choose a reason for hiding this comment

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

I left it in buildPreset to prevent running getTargets method for each item from pluginList

@hzoo hzoo merged commit d3d63e8 into babel:master Oct 13, 2016
@hzoo
Copy link
Member

hzoo commented Oct 13, 2016

Ok great, looks good!! 🎉

@chicoxyzzy
Copy link
Member

chicoxyzzy commented Nov 2, 2016

Do we really need possibility to mention browsers directly in targets? AFAIK browserlist can handle this.

Result of

{
  targets: {
    chrome: 50,
    safari: 10,
    browsers: ['last 2 chrome versions', 'last 2 safari versions']
  }
}

is equal to result of

{
  targets: {
    browsers: ['chrome 50', 'safari 10']
  }
}

Example: http://browserl.ist/?q=last+2+versions%2C+not+ie+%3C+11%2C+chrome+5
Do we need this only for Node.js?

@hzoo
Copy link
Member

hzoo commented Nov 2, 2016

I guess it depends on what browserlist supports compared to the compat-table - yeah node?

@ianwremmel
Copy link

It looks like autoprefixer is now recommending a browserlist file in the project root rather than configure tools directly. Does this support that?

@hzoo
Copy link
Member

hzoo commented Nov 5, 2016

Nope it's just a string atm - there #26 open though!

@ianwremmel
Copy link

gotcha. thanks @hzoo!

This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants