This repository has been archived by the owner. It is now read-only.

Add option to specify browserslist config file #51

Closed
wants to merge 6 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@p-jackson

p-jackson commented Nov 22, 2016

Potential fix for #26

The issue talks about using the appropriate browserslist file for each source file. But as was pointed out (and as far as I know) presets can't work in a per-file kind of way.
However the browserslist package also supports specifying a path to the actual config file, which would be enough for something like create-react-app since it knows where the project's root folder is.

I thought it was best to make it an absolute path since this preset could be anywhere compared to the user project's root folder.
Very happy if someone can think of a better name for the option 馃槃

Let me know if I've misused the fixtures folder, or if there's a preferred way to mock the filesystem.

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Nov 22, 2016

Another way I thought this could be fixed was to start at __dirname (in lib/index.js) and walk down the folder structure looking for a browserslist file, just like browserslist itself does. This way we'd eventually find the user project's root folder. The problem I saw with that was if this preset was a transitive dependency in a nested folder (like in npm 2) it might encounter a different browserslist file along the way.

Having a file path as a preset option doesn't seem very elegant to me, but it seems like the only way to make it work every time.

p-jackson commented Nov 22, 2016

Another way I thought this could be fixed was to start at __dirname (in lib/index.js) and walk down the folder structure looking for a browserslist file, just like browserslist itself does. This way we'd eventually find the user project's root folder. The problem I saw with that was if this preset was a transitive dependency in a nested folder (like in npm 2) it might encounter a different browserslist file along the way.

Having a file path as a preset option doesn't seem very elegant to me, but it seems like the only way to make it work every time.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 2, 2016

Member

Sorry will look at this again soon

Member

hzoo commented Dec 2, 2016

Sorry will look at this again soon

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 9, 2016

Member

Very happy if someone can think of a better name for the option 馃槃

Haha yeah I was wondering that too..

browserslistConfigPath? I think what you have is fine and straightforward now that I've the time to actually look at this! looks great.

Member

hzoo commented Dec 9, 2016

Very happy if someone can think of a better name for the option 馃槃

Haha yeah I was wondering that too..

browserslistConfigPath? I think what you have is fine and straightforward now that I've the time to actually look at this! looks great.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 9, 2016

Member

I'l fix the merge conflicts and merge

Member

hzoo commented Dec 9, 2016

I'l fix the merge conflicts and merge

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 9, 2016

Member

Although now not sure how to test with a fixture right..

will postpone for a bit.

Member

hzoo commented Dec 9, 2016

Although now not sure how to test with a fixture right..

will postpone for a bit.

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Dec 10, 2016

Had a go at adding a fixture for browserslistConfigPath option. Switched fixtures to use the babel-helper-transform-fixture-test-runner package because it lets you adjust options at runtime, which is needed because browserslistConfigPath needs to be absolute.

I used <fixture_path> as the pattern so that people reading the source don't mistakenly think path substitution is a supported feature of babel-preset-env.

p-jackson commented Dec 10, 2016

Had a go at adding a fixture for browserslistConfigPath option. Switched fixtures to use the babel-helper-transform-fixture-test-runner package because it lets you adjust options at runtime, which is needed because browserslistConfigPath needs to be absolute.

I used <fixture_path> as the pattern so that people reading the source don't mistakenly think path substitution is a supported feature of babel-preset-env.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 10, 2016

Member

Nice work on the fixture tests!

I guess it's interesting that this means if you use a regular .babelrc file or in package.json you won't be able to use the browserslistConfigPath option because you would probably do a relative path right?

Member

hzoo commented Dec 10, 2016

Nice work on the fixture tests!

I guess it's interesting that this means if you use a regular .babelrc file or in package.json you won't be able to use the browserslistConfigPath option because you would probably do a relative path right?

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Dec 10, 2016

Member

browserlists has more options that could be useful for us in the future. (ex: stats).
What about put it to the browserslistConfig: {path: 'configPath'} to be able extending it in the future?

Member

yavorsky commented Dec 10, 2016

browserlists has more options that could be useful for us in the future. (ex: stats).
What about put it to the browserslistConfig: {path: 'configPath'} to be able extending it in the future?

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Dec 11, 2016

Yes you're right, I've only really been thinking about the API case, where it's easy to specify an absolute path. If the path were relative where should it be relative to? Relative to the .babelrc or package.json file that specifies the preset options?

I'm worried that having to specify a path is making this feature feel awkward. Someone who uses browserslist files would naturally assume they can put multiple files and different points in the folder structure, but this makes you choose one config for your project. Which doesn't give much benefit over existing babel-preset-env options.

I've been mainly thinking of the CRA case, and as was pointed out in facebook/create-react-app#892 they can just do their own config without a browserslist file.

I think what I'm trying to say is I'd rather make a change to babel-core so it gives preset factories the file they're making the preset for so this can be done "right".

p-jackson commented Dec 11, 2016

Yes you're right, I've only really been thinking about the API case, where it's easy to specify an absolute path. If the path were relative where should it be relative to? Relative to the .babelrc or package.json file that specifies the preset options?

I'm worried that having to specify a path is making this feature feel awkward. Someone who uses browserslist files would naturally assume they can put multiple files and different points in the folder structure, but this makes you choose one config for your project. Which doesn't give much benefit over existing babel-preset-env options.

I've been mainly thinking of the CRA case, and as was pointed out in facebook/create-react-app#892 they can just do their own config without a browserslist file.

I think what I'm trying to say is I'd rather make a change to babel-core so it gives preset factories the file they're making the preset for so this can be done "right".

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Dec 11, 2016

Member

Project's root and node's working directory is often the same, so why not to use process.cwd for making relative (to the root) paths:
path.join(process.cwd(), browserslistConfigPath)

Maybe there are some pitfalls, but just an option...

Member

yavorsky commented Dec 11, 2016

Project's root and node's working directory is often the same, so why not to use process.cwd for making relative (to the root) paths:
path.join(process.cwd(), browserslistConfigPath)

Maybe there are some pitfalls, but just an option...

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Dec 14, 2016

How do other tools that leverage browserslist accomplish this? Ideally, we should just follow that prior art.

mikesherov commented Dec 14, 2016

How do other tools that leverage browserslist accomplish this? Ideally, we should just follow that prior art.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 14, 2016

Member

Yeah good idea, from https://github.com/postcss/autoprefixer#browsers

If you don鈥檛 provide the browsers option, Browserslist will try to find the browserslist config in parent dirs. https://github.com/ai/browserslist/blob/d9c448c3400b88df4949e592ae95d661719000e3/index.js#L236-L254

cc @ai

Member

hzoo commented Dec 14, 2016

Yeah good idea, from https://github.com/postcss/autoprefixer#browsers

If you don鈥檛 provide the browsers option, Browserslist will try to find the browserslist config in parent dirs. https://github.com/ai/browserslist/blob/d9c448c3400b88df4949e592ae95d661719000e3/index.js#L236-L254

cc @ai

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Dec 14, 2016

Having a option with browserslist config is not so clear. It is easy for other developers to miss it. Some other tools (like Stylelint) could miss it too.

In Autoprefixer was have no option for config.

But I understand that many developers want to have configs/ dir and move many files from root from them.

@hzoo when Autoprefixer users ask me about config path option, I told them about .babelrc 鈥 that Babel has no option, it always take it from file path. Why you have no option there?

BTW, package.json support in Browserslist is almost finished. It could clean root from browserslist config.

ai commented Dec 14, 2016

Having a option with browserslist config is not so clear. It is easy for other developers to miss it. Some other tools (like Stylelint) could miss it too.

In Autoprefixer was have no option for config.

But I understand that many developers want to have configs/ dir and move many files from root from them.

@hzoo when Autoprefixer users ask me about config path option, I told them about .babelrc 鈥 that Babel has no option, it always take it from file path. Why you have no option there?

BTW, package.json support in Browserslist is almost finished. It could clean root from browserslist config.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 14, 2016

Member

Yeah that's what I was thinking - just didn't know how it worked but seems to be the same as .babelrc?

Member

hzoo commented Dec 14, 2016

Yeah that's what I was thinking - just didn't know how it worked but seems to be the same as .babelrc?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Dec 14, 2016

@hzoo yeap, I like .babelrc behavior.

But, maybe we (as community) should support some .config/ dir. So if we processing src/test.js we tryt to find config in src/<config>, src/.config/<config>, <config>, .config/<config>.

Developers don鈥檛 want to have everything in root file. Having a option is always a bad UX :D. Maybe all tools (PostCSS, Browserslist, Babel) should agree about some config directory?

ai commented Dec 14, 2016

@hzoo yeap, I like .babelrc behavior.

But, maybe we (as community) should support some .config/ dir. So if we processing src/test.js we tryt to find config in src/<config>, src/.config/<config>, <config>, .config/<config>.

Developers don鈥檛 want to have everything in root file. Having a option is always a bad UX :D. Maybe all tools (PostCSS, Browserslist, Babel) should agree about some config directory?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 14, 2016

Member

That will be a huge change haha, not sure how to convince everyone to do that but we can (outside of this issue though)

Member

hzoo commented Dec 14, 2016

That will be a huge change haha, not sure how to convince everyone to do that but we can (outside of this issue though)

@mikesherov

This comment has been minimized.

Show comment
Hide comment
@mikesherov

mikesherov Dec 14, 2016

@ai check out this discussion on changing .config away from the root: h5bp/lazyweb-requests#151

TL; DR: lots of tool devs prefer root (myself included). Not going to really find enough people to agree.

mikesherov commented Dec 14, 2016

@ai check out this discussion on changing .config away from the root: h5bp/lazyweb-requests#151

TL; DR: lots of tool devs prefer root (myself included). Not going to really find enough people to agree.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Dec 14, 2016

@hzoo OK, I tried :D.

ai commented Dec 14, 2016

@hzoo OK, I tried :D.

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Dec 15, 2016

I agree with the sentiments here that .babelrc and autoprefixer behaviour are better for users. I also like what @ai said about options 馃槃.
I'm still keen to try fixing #26, but I think the first step would be submitting a PR to babel-core because as far as I'm aware babel-preset-env needs changes there before it can implement the .babelrc behaviour.
@hzoo I'm happy for this PR to be closed and we try a different approach if you're happy?

p-jackson commented Dec 15, 2016

I agree with the sentiments here that .babelrc and autoprefixer behaviour are better for users. I also like what @ai said about options 馃槃.
I'm still keen to try fixing #26, but I think the first step would be submitting a PR to babel-core because as far as I'm aware babel-preset-env needs changes there before it can implement the .babelrc behaviour.
@hzoo I'm happy for this PR to be closed and we try a different approach if you're happy?

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Dec 15, 2016

Member

Ok yeah maybe we should try finding the browserslist file instead. Would you like to make that pr @p-jackson? What change do we need to do in babel-core?

Member

hzoo commented Dec 15, 2016

Ok yeah maybe we should try finding the browserslist file instead. Would you like to make that pr @p-jackson? What change do we need to do in babel-core?

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Dec 16, 2016

I think this is the line that calls the function that generates the preset, and I think the path of the current file being processed should be passed here too. It may be available on the this.options object?

p-jackson commented Dec 16, 2016

I think this is the line that calls the function that generates the preset, and I think the path of the current file being processed should be passed here too. It may be available on the this.options object?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Dec 23, 2016

I forgot to mention that there is a official way to specify Browserslist config :). BROWSERSLIST_CONFIG environment variable.

BROWSERSLIST_CONFIG=.config/browserslist npm run build

I like environment variables because it will be shared in all tasks in current process.

ai commented Dec 23, 2016

I forgot to mention that there is a official way to specify Browserslist config :). BROWSERSLIST_CONFIG environment variable.

BROWSERSLIST_CONFIG=.config/browserslist npm run build

I like environment variables because it will be shared in all tasks in current process.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Dec 23, 2016

Also Browserslist 1.5 was released so right now you can specify browsers in package.json to reduce config files in project root:

{
  "private": true,
  "dependencies": {
    "autoprefixer": "^6.5.4"
  },
  "browserslist": [
    "> 1%",
    "last 2 versions"
  ]
}

ai commented Dec 23, 2016

Also Browserslist 1.5 was released so right now you can specify browsers in package.json to reduce config files in project root:

{
  "private": true,
  "dependencies": {
    "autoprefixer": "^6.5.4"
  },
  "browserslist": [
    "> 1%",
    "last 2 versions"
  ]
}
@jrencz

This comment has been minimized.

Show comment
Hide comment
@jrencz

jrencz Jan 3, 2017

I actually tried to implement it myself before I found this PR.

My approach was:

  1. let there be targets.browserslist config which may be true or false. Defaults to true (IDK if that conforms to the config policy or not, probably not, but let me explain)
  2. if true: call browserslist() (without arguments). It will produce its result. Process it with getLowestVersions. if false start with an empty object.
  3. then apply current algorithm for targets.browsers but instead of returning: merge it with what browserslist returned
  4. merge queryBrowsers and targetOps (always: if queryBrowsers is empty object literal it's a noop AFAIK)
var queryBrowsers = targetOps.browserslist ?
    getLowestVersions(browserslist()) :
    {};

var browserOpts = targetOps.browsers;
if (isBrowsersQueryValid(browserOpts)) {
    Object.assign(queryBrowsers, getLowestVersions(browserslist(browserOpts)))
}

return mergeBrowsers(queryBrowsers, targetOps);

This way it seems to be much simpler - babel-preset-env does not take care of internal browserslist behavior of where to get the config from if query is not given.

I just wanted to ask you for voicing your opinion on this approach before I prepare a PR: all I have is just working implementation using code above. If you like it I'll prepare a PR.

jrencz commented Jan 3, 2017

I actually tried to implement it myself before I found this PR.

My approach was:

  1. let there be targets.browserslist config which may be true or false. Defaults to true (IDK if that conforms to the config policy or not, probably not, but let me explain)
  2. if true: call browserslist() (without arguments). It will produce its result. Process it with getLowestVersions. if false start with an empty object.
  3. then apply current algorithm for targets.browsers but instead of returning: merge it with what browserslist returned
  4. merge queryBrowsers and targetOps (always: if queryBrowsers is empty object literal it's a noop AFAIK)
var queryBrowsers = targetOps.browserslist ?
    getLowestVersions(browserslist()) :
    {};

var browserOpts = targetOps.browsers;
if (isBrowsersQueryValid(browserOpts)) {
    Object.assign(queryBrowsers, getLowestVersions(browserslist(browserOpts)))
}

return mergeBrowsers(queryBrowsers, targetOps);

This way it seems to be much simpler - babel-preset-env does not take care of internal browserslist behavior of where to get the config from if query is not given.

I just wanted to ask you for voicing your opinion on this approach before I prepare a PR: all I have is just working implementation using code above. If you like it I'll prepare a PR.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 3, 2017

@jrencz I think it will be better to have same logic for all frontend tools in browserslist config. Having different logic in babel-preset-env and in Autoprefixer with Stylelint will be be unexpected for users.

ai commented Jan 3, 2017

@jrencz I think it will be better to have same logic for all frontend tools in browserslist config. Having different logic in babel-preset-env and in Autoprefixer with Stylelint will be be unexpected for users.

@jrencz

This comment has been minimized.

Show comment
Hide comment
@jrencz

jrencz Jan 3, 2017

@ai I'm not sure if I get you correctly.

Correct me please if I'm wrong but as far as I can see currently the only way to use browserslist syntax in babel-preset-env is to pass target.browsers being either a string or array. It never reads the browserslist file even if it exists because if target.browsers is either not defined or not valid then browserslist function is not run at all.

jrencz commented Jan 3, 2017

@ai I'm not sure if I get you correctly.

Correct me please if I'm wrong but as far as I can see currently the only way to use browserslist syntax in babel-preset-env is to pass target.browsers being either a string or array. It never reads the browserslist file even if it exists because if target.browsers is either not defined or not valid then browserslist function is not run at all.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 3, 2017

@jrencz I don鈥檛 use, so I can not be sure :(.

But yes, right now babel-preset-env uses Browserslist in different way :(.

ai commented Jan 3, 2017

@jrencz I don鈥檛 use, so I can not be sure :(.

But yes, right now babel-preset-env uses Browserslist in different way :(.

@amilajack

This comment has been minimized.

Show comment
Hide comment
@amilajack

amilajack Jan 6, 2017

Is there interest in adding browserslist in package.json? eslint-plugin-compat will support browserslist in package.json and separate browserslist config file.

amilajack commented Jan 6, 2017

Is there interest in adding browserslist in package.json? eslint-plugin-compat will support browserslist in package.json and separate browserslist config file.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 7, 2017

@amilajack wow, great to see that eslint-plugin-compat supports Browserslist too :).

browserslist in package.json works in Browserslist 1.5 (if I understand your question correctly).

ai commented Jan 7, 2017

@amilajack wow, great to see that eslint-plugin-compat supports Browserslist too :).

browserslist in package.json works in Browserslist 1.5 (if I understand your question correctly).

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Jan 7, 2017

Member

@amilajack Current discussion: #108. In general, yes. I think we could (and going to) minimize config files for different things to a single line. Node still an issue, but temporary.

Member

yavorsky commented Jan 7, 2017

@amilajack Current discussion: #108. In general, yes. I think we could (and going to) minimize config files for different things to a single line. Node still an issue, but temporary.

@amilajack

This comment has been minimized.

Show comment
Hide comment
@amilajack

amilajack Jan 7, 2017

I think a better name, would be targets instead of browserslist. There have been a lot of requests to support node. Would be kind of weird if node was listed under browserslist. Thoughts on this?

Also in this comment, Dan describes using browsers instead of browserslist. Which should eslint-plugin-compat support? Personally, I think we should call it targets instead.

amilajack commented Jan 7, 2017

I think a better name, would be targets instead of browserslist. There have been a lot of requests to support node. Would be kind of weird if node was listed under browserslist. Thoughts on this?

Also in this comment, Dan describes using browsers instead of browserslist. Which should eslint-plugin-compat support? Personally, I think we should call it targets instead.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 7, 2017

@amilajack browserslist refer to tool and it's syntax.

Autoprefixer, Stylelint and doiuse (eslint-plugin-compat for CSS) works only with browserslist.

targets is too common word for package.json. There are a lot of "targets": build targets for example.

This is a reason why I used browserslist in package.json. Using package name is safe from name conflicts with other frontend build tools.

ai commented Jan 7, 2017

@amilajack browserslist refer to tool and it's syntax.

Autoprefixer, Stylelint and doiuse (eslint-plugin-compat for CSS) works only with browserslist.

targets is too common word for package.json. There are a lot of "targets": build targets for example.

This is a reason why I used browserslist in package.json. Using package name is safe from name conflicts with other frontend build tools.

@amilajack

This comment has been minimized.

Show comment
Hide comment
@amilajack

amilajack Jan 7, 2017

I see. Also what's the recommended way of importing the user's package.json file? How do babel-preset-env and other plugins do this? Here's how eslint-plugin-compat does this at the moment.

amilajack commented Jan 7, 2017

I see. Also what's the recommended way of importing the user's package.json file? How do babel-preset-env and other plugins do this? Here's how eslint-plugin-compat does this at the moment.

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 7, 2017

@amilajack why you don't want just use browserslist() to be sure about Autoprefixer compatibility?

ai commented Jan 7, 2017

@amilajack why you don't want just use browserslist() to be sure about Autoprefixer compatibility?

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Jan 8, 2017

@hzoo I've had a trouble getting a PR for babel together that would facilitate this. I've had trouble getting the unit tests to run cleanly (could be because I'm using Bash on Windows). I also struggled understand where the code should go. Is it something to do with the "config chain"? I was able to work it out.

The farthest I've got is this commit, but I don't know if it's too na茂ve, and as I said I couldn't get the tests to work, so haven't submitted it as a PR to babel.

But I still think the approach of changing babel first before changing babel-preset-env is still a good one, and is supported by the comments that have been posted here (i.e. use browserslist to do the logic so it picks up changes like using config from package.json "for free"). But it's just not a nut I've been able to crack unfortunately.

p-jackson commented Jan 8, 2017

@hzoo I've had a trouble getting a PR for babel together that would facilitate this. I've had trouble getting the unit tests to run cleanly (could be because I'm using Bash on Windows). I also struggled understand where the code should go. Is it something to do with the "config chain"? I was able to work it out.

The farthest I've got is this commit, but I don't know if it's too na茂ve, and as I said I couldn't get the tests to work, so haven't submitted it as a PR to babel.

But I still think the approach of changing babel first before changing babel-preset-env is still a good one, and is supported by the comments that have been posted here (i.e. use browserslist to do the logic so it picks up changes like using config from package.json "for free"). But it's just not a nut I've been able to crack unfortunately.

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Mar 29, 2017

Member

Closing in favor of #161

Member

hzoo commented Mar 29, 2017

Closing in favor of #161

@hzoo hzoo closed this Mar 29, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.