Pass default webpack config as third argument in Full Control Mode #2796
Conversation
|
@thomasbertet can we make image tests fail on CI when some screenshots are added during it (that is, if they were missing before running the test)? |
Codecov Report
@@ Coverage Diff @@
## master #2796 +/- ##
==========================================
+ Coverage 35.83% 35.87% +0.04%
==========================================
Files 427 427
Lines 9422 9411 -11
Branches 985 974 -11
==========================================
Hits 3376 3376
- Misses 5363 5394 +31
+ Partials 683 641 -42
Continue to review full report at Codecov.
|
| ```js | ||
| const path = require('path'); | ||
|
|
||
| // load the default config generator. | ||
| const genDefaultConfig = require('@storybook/react/dist/server/config/defaults/webpack.config.js'); |
There was a problem hiding this comment.
Shouldn't it be mentioned in docs as a deprecated feature?
There was a problem hiding this comment.
IDK, just have seen people leave deprecated features with some "deprecated" labels on them... But it's not critical to me 👍
There was a problem hiding this comment.
I think if we don't want people to do something we delete it from the docs :)
Unless it's something that we think a lot of people are already doing, in which case a deprecated note could help in some cases. But probably people who are already doing something are unlikely to come back to the docs, so really I don't think it gains much.
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| test: /\.json$/, | ||
| include: includePaths, |
There was a problem hiding this comment.
So these includes weren't really needed?
There was a problem hiding this comment.
Yes, we have only js files in those directories currently
| @@ -1,63 +1,7 @@ | |||
| import autoprefixer from 'autoprefixer'; | |||
There was a problem hiding this comment.
When I checked this config before, it was a bit different from the default config of the other apps. Are those differences minor? For example, is it safe to change "loaders" to "rules"?
There was a problem hiding this comment.
loaders field is deprecated in webpack. Technically, it's a breaking change, but I can't really imagine why would someone use a custom webpack config in RN storybook. It doesn't apply to stories themselves anyway.
Generally, the differences are there because changes to configs in react and vue weren't applied here
There was a problem hiding this comment.
I guess we need to factor our the RN manager config into @storybook/core too. Maybe given the above it is easier than you thought @igor-dv?
| return customConfig( | ||
| applyAngularCliWebpackConfig(config, cliWebpackConfigOptions), | ||
| configType, | ||
| defaultConfig |
There was a problem hiding this comment.
So in case of full control mode, there is a first parameter that is extended with angular-cli's config, and the default config which is extended as well. Is this expected?
There was a problem hiding this comment.
Given that you actually pick one or another, yes
See #2788 (comment)
When reviewing, please pay specific attention to angular and RN changes