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 other type of webpack configs #3785

Merged
merged 5 commits into from
Jul 1, 2018
Merged

Conversation

Ailrun
Copy link
Contributor

@Ailrun Ailrun commented Jun 22, 2018

Issue: #3780.

What I did

Add interpret package to support other type (.ts, .babel.js, ...) of webpack config files.

How to test

Install ts-node and typescript in official-storybook in example directory, and change webpack.config.js to webpack.config.ts, and finally change requires to imports.

Is this testable with Jest or Chromatic screenshots? No
Does this need a new example in the kitchen sink apps? No
Does this need an update to the documentation? I'm not sure.

@codecov
Copy link

codecov bot commented Jun 22, 2018

Codecov Report

Merging #3785 into master will decrease coverage by 0.31%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3785      +/-   ##
==========================================
- Coverage   40.98%   40.66%   -0.32%     
==========================================
  Files         453      454       +1     
  Lines        5100     5139      +39     
  Branches      861      872      +11     
==========================================
  Hits         2090     2090              
- Misses       2493     2521      +28     
- Partials      517      528      +11
Impacted Files Coverage Δ
lib/core/src/server/loadCustomWebpackConfig.js 0% <0%> (ø)
lib/core/src/server/config.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e3e1d27...230b257. Read the comment docs.

@Ailrun
Copy link
Contributor Author

Ailrun commented Jun 22, 2018

@Hypnosphi It looks like the check fails because of yarn registry. Could you rerun it?

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

I'm not very comfortable with copy-pasting the code from webpack sources, but it's probably the best we can do now.

Can you please open an issue for https://github.com/webpack/webpack-cli and ask them to make config resolution logic available for external usage? Babel does a similar thing in v7: babel/babel#7472

@Ailrun
Copy link
Contributor Author

Ailrun commented Jun 23, 2018

@Hypnosphi the problem is that storybook could accept different type of configurations (even the file names are same with webpack one, storybook takes different function like custom config)

@@ -58,6 +59,87 @@ function informAboutCustomConfig(defaultConfigName) {
logger.info(`=> Using default webpack setup based on "${defaultConfigName}".`);
}

// Copied and modified from
Copy link
Member

Choose a reason for hiding this comment

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

Please extract all these to a separate file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I will extract them into webpack_config. Is it what you expect?

@Hypnosphi
Copy link
Member

Yes, we only need to resolve the filename

@Ailrun
Copy link
Contributor Author

Ailrun commented Jun 25, 2018

@igor-dv Could you check this again?

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

Let's rename webpack_config to the loadCustomWebpackConfig and we are OK.

@Ailrun
Copy link
Contributor Author

Ailrun commented Jun 26, 2018

@igor-dv Is there any reason for snake_case file name and camelCase file name? I mean, babel_config use snake_case, where loadCustomWebpackConfig use camelCase.

@igor-dv
Copy link
Member

igor-dv commented Jun 27, 2018

I am not surer about the convention, but it's more about maintenance/readability. I would also change the babel_config to loadBabelConfig because it confuses.

@igor-dv
Copy link
Member

igor-dv commented Jul 1, 2018

Also related to an older issue - #3125.

@nickserv
Copy link

nickserv commented Sep 19, 2018

I think I found a regression:

info @storybook/react v4.0.0-alpha.21
info
info => Loading presets
WARN => File /Users/nick/Repos/ui/.storybook/webpack.config.ts is detected
WARN    but impossible to import loader for .ts

@87Hz
Copy link

87Hz commented Jun 28, 2019

I think I found a regression:

info @storybook/react v4.0.0-alpha.21
info
info => Loading presets
WARN => File /Users/nick/Repos/ui/.storybook/webpack.config.ts is detected
WARN    but impossible to import loader for .ts

Do not forget to install @babel/register.

@kristojorg
Copy link

kristojorg commented Feb 5, 2020

@87Hz What do you mean? Why am I installing that and where would I use it?

I'm getting the same error as @nickmccurdy but with .storybook/main.ts

@87Hz
Copy link

87Hz commented Feb 5, 2020

@87Hz What do you mean? Why am I installing that and where would I use it?

I'm getting the same error as @nickmccurdy but with .storybook/main.ts

That is to ensure .ts files are properly transcompiled by Babel. You do not need to use it anywhere, it is handled by Babel.

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

Successfully merging this pull request may close these issues.

6 participants