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

Send file path to browserslist #26

Closed
ai opened this Issue Oct 13, 2016 · 25 comments

Comments

Projects
None yet
9 participants
@ai

ai commented Oct 13, 2016

Browserslist could use browserslist config file. It is useful, because many different tools will share same config (Autoprefixer, Stylelint, cssnext, doiuse).

But to enable a config, you need to pass a path for current file:

browserslist(opts.browsers, { path: opts.file })
@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Oct 13, 2016

Member

I guess we could check the file but given a preset could be somewhere else I'm not sure.

Also relevant facebook/create-react-app#892

Member

hzoo commented Oct 13, 2016

I guess we could check the file but given a preset could be somewhere else I'm not sure.

Also relevant facebook/create-react-app#892

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Nov 10, 2016

Member

If someone wants to make a PR

Member

hzoo commented Nov 10, 2016

If someone wants to make a PR

@Kovensky

This comment has been minimized.

Show comment
Hide comment
@Kovensky

Kovensky Nov 10, 2016

Member

I tried looking into it, but the preset doesn't have access to useful information about the .babelrc or the file being processed 😕

The preset is called with a context argument, but it turns out that the context is actually just the node api namespace, which you can easily import yourself, instead of an actual context, so there's not even any way to know which .babelrc is being used, let alone which file is being transformed 😢

Member

Kovensky commented Nov 10, 2016

I tried looking into it, but the preset doesn't have access to useful information about the .babelrc or the file being processed 😕

The preset is called with a context argument, but it turns out that the context is actually just the node api namespace, which you can easily import yourself, instead of an actual context, so there's not even any way to know which .babelrc is being used, let alone which file is being transformed 😢

@p-jackson

This comment has been minimized.

Show comment
Hide comment
@p-jackson

p-jackson Nov 22, 2016

I made a PR, but having slept on it I feel like the original suggestion would be much more elegant.

Looks like it'd be possible to change babel-core to pass the current file to the preset. Do you think babel-core would be open to a PR like that?

p-jackson commented Nov 22, 2016

I made a PR, but having slept on it I feel like the original suggestion would be much more elegant.

Looks like it'd be possible to change babel-core to pass the current file to the preset. Do you think babel-core would be open to a PR like that?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Jan 4, 2017

@hzoo I changed Browserslist to fit Babel and CRA requirements. Now it is your turn to enable a config (not just a option).

ai commented Jan 4, 2017

@hzoo I changed Browserslist to fit Babel and CRA requirements. Now it is your turn to enable a config (not just a option).

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Feb 19, 2017

Hey @ai, I'd like to jump in and get this working on the Babel side. I'm not sure what you mean by your last comment though - could you elaborate?

motiz88 commented Feb 19, 2017

Hey @ai, I'd like to jump in and get this working on the Babel side. I'm not sure what you mean by your last comment though - could you elaborate?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 19, 2017

@motiz88 I mean that it is Babel turn to set path option in Browserslist to load config even if browsers option is missed in .babelrc.

ai commented Feb 19, 2017

@motiz88 I mean that it is Babel turn to set path option in Browserslist to load config even if browsers option is missed in .babelrc.

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Feb 19, 2017

babel/babel#4834 would enable this nicely.

Related: It's worth considering how this fits together with #161, which supports specifying an explicit path in the preset options. Possible behaviours off the top of my head:

  1. If an explicit path is set, never look up any per-file config via browserslist.
  2. If an explicit path is set, try it first; If a browserslist config can't be found that way, fall back to looking at each file's path.
  3. Lookup browserslist config relative to each file, and only if that fails - fall back to the explicit path (if any).

motiz88 commented Feb 19, 2017

babel/babel#4834 would enable this nicely.

Related: It's worth considering how this fits together with #161, which supports specifying an explicit path in the preset options. Possible behaviours off the top of my head:

  1. If an explicit path is set, never look up any per-file config via browserslist.
  2. If an explicit path is set, try it first; If a browserslist config can't be found that way, fall back to looking at each file's path.
  3. Lookup browserslist config relative to each file, and only if that fails - fall back to the explicit path (if any).
@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 19, 2017

I think, that if user set config path, we should use only this config.

ai commented Feb 19, 2017

I think, that if user set config path, we should use only this config.

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

We already have #161. It accepts all logic provided by browserslist, including path. The only thing it makes extraordinary is set targets specified in preset-env options as default browsers for browserslist to prevent default browsers if we hadn't set browsers in targets. It's temporary and I don't think it could affect workflow somehow.

Member

yavorsky commented Feb 19, 2017

We already have #161. It accepts all logic provided by browserslist, including path. The only thing it makes extraordinary is set targets specified in preset-env options as default browsers for browserslist to prevent default browsers if we hadn't set browsers in targets. It's temporary and I don't think it could affect workflow somehow.

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Feb 19, 2017

@yavorsky One thing #161 doesn't do (please correct me if I'm wrong) is allow babel-preset-env to pick up an existing Browserslist config file without requiring path to be set. This is what I'm getting at here, in keeping with the idea that

All tools that rely on Browserslist will find its config automatically [...in package.json or browserslist]

motiz88 commented Feb 19, 2017

@yavorsky One thing #161 doesn't do (please correct me if I'm wrong) is allow babel-preset-env to pick up an existing Browserslist config file without requiring path to be set. This is what I'm getting at here, in keeping with the idea that

All tools that rely on Browserslist will find its config automatically [...in package.json or browserslist]

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

@motiz88 It handles path option - https://github.com/babel/babel-preset-env/pull/161/files#diff-1fdf421c05c1140f6d71444ea2b27638R119, so it's possible to set:

["env", {
  "path": "path/to/config"
}]

where path/to/config - directory with package.json or browserslist's config.
You can dive deeper in the fixtures.

Without path option it would start to search in process.cwd(). Also, it accepts BROWSERSLIST_CONFIG var.
Actually, we don't handle the behaviour related to config search. We only process and pass correct arguments to the browserslist (browsers, path, defaults) and let it do the rest of job.

This PR isn't merged yet, so it's cool to see more propositions about technical realization, etc. 😉

Member

yavorsky commented Feb 19, 2017

@motiz88 It handles path option - https://github.com/babel/babel-preset-env/pull/161/files#diff-1fdf421c05c1140f6d71444ea2b27638R119, so it's possible to set:

["env", {
  "path": "path/to/config"
}]

where path/to/config - directory with package.json or browserslist's config.
You can dive deeper in the fixtures.

Without path option it would start to search in process.cwd(). Also, it accepts BROWSERSLIST_CONFIG var.
Actually, we don't handle the behaviour related to config search. We only process and pass correct arguments to the browserslist (browsers, path, defaults) and let it do the rest of job.

This PR isn't merged yet, so it's cool to see more propositions about technical realization, etc. 😉

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 19, 2017

@yavorsky in next major Browserslist will not look in process.cwd() on missed path.

path option in Browserslist should be set not to config, but to processed JS file.

ai commented Feb 19, 2017

@yavorsky in next major Browserslist will not look in process.cwd() on missed path.

path option in Browserslist should be set not to config, but to processed JS file.

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

@ai But browserslist currently use path option only to find a config file?

If a query is missing, Browserslist will look for a config file. You can provide a path option (that can be a file) to find the config file relatively to it.
Member

yavorsky commented Feb 19, 2017

@ai But browserslist currently use path option only to find a config file?

If a query is missing, Browserslist will look for a config file. You can provide a path option (that can be a file) to find the config file relatively to it.
@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 19, 2017

Yeap, but semantically path is a path to processed file (JS file for Babel or CSS for Autoprefixer).

So in Autoprefixer/Stylelint, user doesn’t need to specify path to config manually. Developer just processes files in Autoprefixer. And Browserslist automatically find config in CSS files parents directories.

ai commented Feb 19, 2017

Yeap, but semantically path is a path to processed file (JS file for Babel or CSS for Autoprefixer).

So in Autoprefixer/Stylelint, user doesn’t need to specify path to config manually. Developer just processes files in Autoprefixer. And Browserslist automatically find config in CSS files parents directories.

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

But in the case of preset-env we haven't any js files on preset building stage. We only have possible process root and config provided by the user. 🤔
@ai What behavior will be if path wasn't specified? No config search at all?

Member

yavorsky commented Feb 19, 2017

But in the case of preset-env we haven't any js files on preset building stage. We only have possible process root and config provided by the user. 🤔
@ai What behavior will be if path wasn't specified? No config search at all?

@ai

This comment has been minimized.

Show comment
Hide comment
@ai

ai Feb 19, 2017

@yavorsky as a short-term solution rename path option to config and reduce UX by asking user to always pass a config path.

As a long-term solution try to change API to allow rebuild preset for every file.

I think users will expect same behaviour as for .babelrc for browserslist.

ai commented Feb 19, 2017

@yavorsky as a short-term solution rename path option to config and reduce UX by asking user to always pass a config path.

As a long-term solution try to change API to allow rebuild preset for every file.

I think users will expect same behaviour as for .babelrc for browserslist.

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

@ai Thanks! 👍 Also I'll play with preset's context and maybe it helps.

Member

yavorsky commented Feb 19, 2017

@ai Thanks! 👍 Also I'll play with preset's context and maybe it helps.

@motiz88

This comment has been minimized.

Show comment
Hide comment
@motiz88

motiz88 Feb 19, 2017

@ai @yavorsky Babel does rebuild the preset for every input file, and as I alluded to above, babel/babel#4834 amends that behaviour to pass the input path as an extra argument to the preset constructor. Hopefully that PR is on its way to being merged on the Babel side.

motiz88 commented Feb 19, 2017

@ai @yavorsky Babel does rebuild the preset for every input file, and as I alluded to above, babel/babel#4834 amends that behaviour to pass the input path as an extra argument to the preset constructor. Hopefully that PR is on its way to being merged on the Babel side.

@yavorsky

This comment has been minimized.

Show comment
Hide comment
@yavorsky

yavorsky Feb 19, 2017

Member

@motiz88 I mean it doesn't support path for babel.transform to use it from loaders/plugins with builders.

Thanks for the link, I really never met or just forgot about this PR. 🎉

Member

yavorsky commented Feb 19, 2017

@motiz88 I mean it doesn't support path for babel.transform to use it from loaders/plugins with builders.

Thanks for the link, I really never met or just forgot about this PR. 🎉

@evilebottnawi

This comment has been minimized.

Show comment
Hide comment
@evilebottnawi

evilebottnawi Apr 14, 2017

Contributor

@yavorsky What is the final implementation algorithm? I want to champion this PR 😄

Contributor

evilebottnawi commented Apr 14, 2017

@yavorsky What is the final implementation algorithm? I want to champion this PR 😄

@stale

This comment has been minimized.

Show comment
Hide comment
@stale

stale bot Jun 14, 2017

This issue has been automatically marked as stale because it has not had recent activity 😴. It will be closed if no further activity occurs. Thank you for your contributions 👌!

stale bot commented Jun 14, 2017

This issue has been automatically marked as stale because it has not had recent activity 😴. It will be closed if no further activity occurs. Thank you for your contributions 👌!

@knynkwl

This comment has been minimized.

Show comment
Hide comment
@knynkwl

knynkwl Sep 2, 2017

Any updates on this?

knynkwl commented Sep 2, 2017

Any updates on this?

@stevewillard

This comment has been minimized.

Show comment
Hide comment
@stevewillard

stevewillard Sep 2, 2017

@knynkwl I think 2.x version supports this feature, so you would need to upgrade to babel 7

stevewillard commented Sep 2, 2017

@knynkwl I think 2.x version supports this feature, so you would need to upgrade to babel 7

@hzoo

This comment has been minimized.

Show comment
Hide comment
@hzoo

hzoo Sep 2, 2017

Member

Done in #161 (correct for v7)

Member

hzoo commented Sep 2, 2017

Done in #161 (correct for v7)

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