Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

Allow using browserlistsrc for targets #1149

Merged
merged 8 commits into from
Oct 4, 2018

Conversation

timkelty
Copy link
Contributor

Resolves #1141

Allows users to pass false to options.targets or options.targets.browsers to have babel-env use a .browserlistsrc file.

Copy link
Member

@eliperelman eliperelman left a comment

Choose a reason for hiding this comment

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

LGTM, @edmorley?

@@ -90,7 +94,7 @@ module.exports = (neutrino, opts = {}) => {
'last 2 iOS versions'
];
}

Copy link
Member

Choose a reason for hiding this comment

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

Can you revert the change to this line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whups

Copy link
Contributor Author

@timkelty timkelty Sep 28, 2018

Choose a reason for hiding this comment

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

Whoopsiedaisy

Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

Thank you for the PR :-)

if (!options.targets.node && !options.targets.browsers) {
if (options.targets === false) {
options.targets = {};
} else if (options.targets.browsers === false) {
Copy link
Member

Choose a reason for hiding this comment

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

For options.targets.browsers to be false, they would have had to set it themselves as false, since targets defaults to {}. For "I want babel to just use its own logic" I think getting people to always use targets: false might make more sense?

Copy link
Contributor Author

@timkelty timkelty Sep 28, 2018

Choose a reason for hiding this comment

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

Yeah - I did it this way because it seemed like options.targets had other props besides browsers that maybe you'd want to set, but still use the browerslistrc. Not totally sure of that's valid though, will have to check when I'm back on a machine: https://babeljs.io/docs/en/babel-preset-env#options

packages/web/index.js Show resolved Hide resolved
@timkelty
Copy link
Contributor Author

timkelty commented Oct 2, 2018

@edmorley I think you're right – we only need to support options.targets as false, not options.targets.browsers.

Updated PR.

I also moved the defaults for the browsers up to the top with the other defaults.

@timkelty
Copy link
Contributor Author

timkelty commented Oct 2, 2018

Actually, if we set the defaults up-top like this, we don't even need to account for false, and have users just pass {} if they want to overrides defaults.

Just passes things directly to @babel/preset-env with an overridable default.
Seem simpler. Thoughts?

@edmorley
Copy link
Member

edmorley commented Oct 4, 2018

Seem simpler. Thoughts?

The preset default options are merged with the ones the user passes using deepmerge, so I think the new implementation will prevent people from overriding. Some tests would confirm and likely be useful edither way :-)

@timkelty
Copy link
Contributor Author

timkelty commented Oct 4, 2018

The preset default options are merged with the ones the user passes using deepmerge, so I think the new implementation will prevent people from overriding

@edmorley right you were :) Reverted that and added some tests.

@edmorley edmorley added this to the v9 milestone Oct 4, 2018
Copy link
Member

@edmorley edmorley left a comment

Choose a reason for hiding this comment

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

This looks great - thank you!

@timkelty timkelty merged commit 66fa422 into neutrinojs:master Oct 4, 2018
@timkelty timkelty deleted the browserlistrc branch October 4, 2018 12:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

Successfully merging this pull request may close these issues.

3 participants