Move more things to core #2788
Move more things to core #2788
Conversation
Codecov Report
@@ Coverage Diff @@
## master #2788 +/- ##
==========================================
+ Coverage 35.51% 35.83% +0.31%
==========================================
Files 434 427 -7
Lines 9501 9422 -79
Branches 987 970 -17
==========================================
+ Hits 3374 3376 +2
+ Misses 5481 5403 -78
+ Partials 646 643 -3
Continue to review full report at Codecov.
|
lib/core/client.js
Outdated
| const client = require('./dist/client').default; | ||
|
|
||
| module.exports = client; | ||
| module.exports.pathToManager = require.resolve('./dist/client/manager'); |
There was a problem hiding this comment.
I would have called this managerPath
| @@ -1,73 +1,5 @@ | |||
| // import webpack from 'webpack'; | |||
| import autoprefixer from 'autoprefixer'; | |||
| import { createDefaultWebpackConfig } from '@storybook/core/server'; | |||
There was a problem hiding this comment.
How about moving the webpack configs into a separate package?
So this becomes much easier:
https://storybook.js.org/configurations/custom-webpack-config/#full-control-mode--default
There was a problem hiding this comment.
What do you mean? Will it be something like @storybook/configs ? TBH, I think we need to make some progress in core extraction because this copypasta across all the apps makes me crazy 😷
There was a problem hiding this comment.
@storybook/webpack-config sounds good to me. We only need to publish the default one, because it's the only one that should be imported from outside
There was a problem hiding this comment.
But maybe it would be an even better solution just to pass genDefaultConfig as third argument to custom webpack config function, so that users could write full control + defaults like this, without any extra imports:
module.exports = (baseConfig, env, genDefaultConfig) => {
const config = genDefaultConfig(baseConfig, env);
// Extend it as you need.
};Or maybe even like this:
module.exports = (baseConfig, env, defaultConfig) => {
// Extend `defaultConfig` as you need.
};There was a problem hiding this comment.
Makes sense to me. It's way much simpler than publishing one more package
There was a problem hiding this comment.
I mean from the user's point of view =)
There was a problem hiding this comment.
I'd rather not create yet another package. @Hypnosphi the proposals above seem pretty reasonable--are they backwards compatible or breaking changes?
There was a problem hiding this comment.
@shilman We should deprecate the old way using utils-deprecate for now, and then remove it in next major
So the answer is yes, it can be backwards-compatible
|
My bad @igor-dv you're right you've made sure this is backwards compatible ✅ |
|
Merge when ready |
Issue: #2636
What I did
(Didn't touch RN)