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

Support for .babelrc.js #2582

Closed
eur00t opened this issue Dec 27, 2017 · 25 comments
Closed

Support for .babelrc.js #2582

eur00t opened this issue Dec 27, 2017 · 25 comments

Comments

@eur00t
Copy link

eur00t commented Dec 27, 2017

Issue details

Currently @storybook/react only looks for Babel configuration in .babelrc file:

https://github.com/storybooks/storybook/blob/d63bf65afa75f2f6f4c0f63b0fedb990a8ea98d0/app/react/src/server/babel_config.js#L48

In @babel/core they support more configuration sources (.babelrc.js, .babelrc, package.json):

https://github.com/babel/babel/blob/7d798952d2a305cf77d24d1707f723f0a162aadd/packages/babel-core/src/config/files/configuration.js#L29

Looks like the functionality of reading Babel configuration is common enough to be extracted into its own package. I guess it's not good enough just to reimplement the same logic in Storybook repo. Or we can just use Babel's internals – though it will be not easy to maintain down the road.

Any thoughts?

Steps to reproduce

  1. Have Babel configuration in .babelrc.js file.
  2. Run @storybook/react with something like start-storybook -p 9001 -c .storybook.
  3. .babelrc.js is ignored

Expected behaviour: Storybook reads Babel configuration from .babelrc.js file.

Please specify which version of Storybook and optionally any affected addons that you're running

  • @storybook/react 3.3.1

Affected platforms

babel-loader 7.1.2
babel-core ^7.0.0-0
@babel/core 7.0.0-beta.34

@psimyn
Copy link
Member

psimyn commented Jan 2, 2018

another example: jest currently reimplements same logic: https://github.com/facebook/jest/blob/master/packages/babel-jest/src/index.js#L29

adding it as an export from babel-core makes sense

@Andarist
Copy link
Contributor

What's your use case for reading the config file on your own? Why don't you leverage built-in mechanism in babel?

@Hypnosphi
Copy link
Member

We need to do some tweaks with config after parsing:
https://github.com/storybooks/storybook/blob/master/app/react/src/server/babel_config.js#L47

@Andarist
Copy link
Contributor

What I have identified (correct me if im wrong) as your use cases:

  • providing a default config in case there is no user config provided
  • extending provided config with babel-plugin-react-docgen

From what I understand the latter could be achieved by simply providing an additional plugin here and let babel merge those (it will do this unless u specify babelrc: false).

Im not sure how to solve first without reimplementing loadBabelConfig though, I'd love to hear when this is needed. When people do not provide their configs that a default config is needed?

@Andarist
Copy link
Contributor

I also see that you duplicate this logic quite a lot between all ./app/* directories. Any reason for this? Seems like most of that code is the same and should be deduplicated - so if we introduce a fix for this issue it ideally would just have to be made in a single file.

@Hypnosphi
Copy link
Member

could be achieved by simply providing an additional plugin here and let babel merge those (it will do this unless u specify babelrc: false).

This sounds like a good solution to me. Would you try to open a PR implementing that?

I also see that you duplicate this logic

Yeah, there's an ongoing work on extracting common parts (#2240), and any help is welcome

@Andarist
Copy link
Contributor

This sounds like a good solution to me. Would you try to open a PR implementing that?

The problem is that it's probably not worth it as there is a "default" config requirement too. I'd like to understand its use cases more first. Other than that, I'm willing to put up a PR fixing this one way or another.

@Hypnosphi
Copy link
Member

The use case for default config is simple: stories files are written in es6 and jsx by default, and they should work even for users that don't have their own Babel config (e.g. the ones who use Create React App)

@eur00t
Copy link
Author

eur00t commented Jan 15, 2018

From what I understand the latter could be achieved by simply providing an additional plugin here and let babel merge those (it will do this unless u specify babelrc: false).

Looks like this merge will not be done by babel itself, but by babel-loader. The problem is the same with it – they also have their own implementation of config read logic (https://github.com/babel/babel-loader/blob/master/src/resolve-rc.js#L5).

What we want, is to be able to find babel configuration for the project in some specified folder. And then internally we can enhance it any way we need. But it's best if original config read logic comes from one of the main @babel/* packages. So if they change something, we don't have to change it also here. (e.g. the original problem in this thread: adding .babelrc.js as a config source).

Looks like they have quite elaborate functionality in babel/packages/babel-core/src/config/, and it's being exported in some way as loadOptions from @babel/core – may be this is what can be used from the outside.

@Hypnosphi
Copy link
Member

Hypnosphi commented Jan 15, 2018

@eur00t you can try to replace the default babel-loader with your own (which can be version 8)

// .strorybook/webpack.config.js

module.exports = baseConfig => {
  baseConfig.module.rules = [{
    test: /\.js$/,
    exclude: /node_modules/,
    loader: 'babel-loader'
  }];
  return baseConfig;
};

If you’re using addon-info, you may need to add babel-plugin-react-docgen manually

@Andarist
Copy link
Contributor

It seems that babel-loader loads the babelrc only to use it as part of the cache key - https://github.com/babel/babel-loader/blob/40552b60dfb1f070dd79d33de0bb799026592e20/src/index.js#L136 .

By default if you specify .babelrc and use CLI with something like:

babel src --out-dir lib --plugins some-plugin

It will use both babelrc and extra plugin. I wouldnt expect this to be a standard behaviour for all ways of calling underlaying JS transform API.

What we want, is to be able to find babel configuration for the project in some specified folder. And then internally we can enhance it any way we need. But it's best if original config read logic comes from one of the main @babel/* packages.

I'd argue that it's better to rely on babel to "enhance" the config, unless there is a reason not to - i.e. if babel doesn't allow for something, but even then you should file an issue for us at babel, so we can discuss possibilities. We'd like to support all reasonable use cases in the core.

Also the problem at the moment will be that we can create a new nice APIs for you to use for babel@7, but its semantics will differ from babel@6 and it might not be perfect to use babel@7 logic to get babel@6 configs - OTOH they might not differ that much.

The use case for default config is simple: stories files are written in es6 and jsx by default, and they should work even for users that don't have their own Babel config (e.g. the ones who use Create React App)

Thanks for info. Will look into how CRA handles babel under the hood.

@eur00t
Copy link
Author

eur00t commented Jan 15, 2018

@Hypnosphi yeap, that's one way of making it work. For now, I just have .storybook/.babelrc, which I use as a static base, and I add dynamic features by extending it in my project's /.babelrc.js.

@eur00t
Copy link
Author

eur00t commented Jan 15, 2018

It seems that babel-loader loads the babelrc only to use it as part of the cache key

(Looks like the same is also in babel-jest – they use it for caching only.)
Yeah, you're right, babel-loader gives options to @babel/core, and then they are merged there. So it's good idea, as you say, to just give it "plugins" and let it merge them.

@stale
Copy link

stale bot commented Mar 1, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

@stale stale bot added the inactive label Mar 1, 2018
@Andarist
Copy link
Contributor

Andarist commented Mar 1, 2018

Oh, I've wanted to contribute to this, but couldn't find time for it, also it might be good to delay implementation until babel@7 gets out, there is a high chance that it will have some nice API for config loading that we could utilize here

@stale stale bot removed the inactive label Mar 1, 2018
@Hypnosphi
Copy link
Member

there is a high chance that it will have some nice API for config loading

Where can I read more about this?

@Andarist
Copy link
Contributor

Andarist commented Mar 4, 2018

babel/babel#7472

@stale
Copy link

stale bot commented Mar 25, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale
Copy link

stale bot commented Apr 24, 2018

Hey there, it's me again! I am going close this issue to help our maintainers focus on the current development roadmap instead. If the issue mentioned is still a concern, please open a new ticket and mention this old one. Cheers and thanks for using Storybook!

@stale stale bot closed this as completed Apr 24, 2018
@Hypnosphi Hypnosphi reopened this Apr 24, 2018
@stale stale bot removed the inactive label Apr 24, 2018
@Hypnosphi Hypnosphi added the todo label Apr 24, 2018
@augbog
Copy link

augbog commented Aug 20, 2018

Hey just curious will this be handled in the Babel 7 upgrade? If not, is the recommendation that if your project uses .babelrc.js, that you define a separate .babelrc file in your storybooks folder for storybooks to pick up? Sorry if this was clarified somewhere else!

@igor-dv
Copy link
Member

igor-dv commented Aug 21, 2018

AFAIK it was not handled. @Hypnosphi fix me if I am wrong.

define a separate .babelrc file in your storybooks folder for storybooks to pick up

As a workaround - yes.

@Hypnosphi
Copy link
Member

Hypnosphi commented Aug 25, 2018

I'd say the best workaround was and still is to have a custom webpack.config.js with your own instance of babel-loader

@jakierice
Copy link

@Hypnosphi one of your original suggestions of putting a .babelrc in the .storybook directory and then having a seperate .babelrc.js for other purposes like testing/build works well for me. Is there plans to officially recommend this in the docs?

@Hypnosphi
Copy link
Member

I haven't heard about such plans

@issue-sh issue-sh bot removed the todo label Aug 27, 2018
@Hypnosphi
Copy link
Member

Released as 4.0.0-alpha.20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants