Support css-modules #4405
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,7 @@ | ||
| import autoprefixer from 'autoprefixer'; | ||
|
|
||
| const cssModuleRegex = /\.module\.css$/; | ||
|
|
||
| export function createDefaultWebpackConfig(storybookBaseConfig) { | ||
| return { | ||
| ...storybookBaseConfig, | ||
|
|
@@ -9,6 +11,7 @@ export function createDefaultWebpackConfig(storybookBaseConfig) { | |
| ...storybookBaseConfig.module.rules, | ||
| { | ||
| test: /\.css$/, | ||
| exclude: cssModuleRegex, | ||
| use: [ | ||
| require.resolve('style-loader'), | ||
| { | ||
|
|
@@ -32,6 +35,20 @@ export function createDefaultWebpackConfig(storybookBaseConfig) { | |
| }, | ||
| ], | ||
| }, | ||
| { | ||
| test: cssModuleRegex, | ||
| loaders: [ | ||
| require.resolve('style-loader'), | ||
| { | ||
| loader: require.resolve('css-loader'), | ||
| options: { | ||
| importLoaders: 1, | ||
| modules: true, | ||
| localIdentName: '[name]__[local]___[hash:base64:5]', | ||
| }, | ||
| }, | ||
| ], | ||
|
There was a problem hiding this comment. we'll still want our default postcss in action here. You could save line 23-34 as There was a problem hiding this comment. Good catch! CRA has a There was a problem hiding this comment. That is why we need to integrate with cra properly, and not mimicking it. There was a problem hiding this comment. Agreed, integrating is the better approach. This is just following how it's currently implemented. |
||
| }, | ||
| { | ||
| test: /\.(svg|ico|jpg|jpeg|png|gif|eot|otf|webp|ttf|woff|woff2)(\?.*)?$/, | ||
| loader: require.resolve('file-loader'), | ||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you meant to keep this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I updated it to use
postcss-preset-env.https://github.com/storybooks/storybook/pull/4405/files/4a6fd1baf91c7f19324bc735c26aa981f4ac8410#diff-ba9b40ce49bd5df0209abd09fc032f58R28
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool - haven't heard of this!
All of the below if for my incoming PR:
So if this is based upon
.browserlistrc... What happens when their coverage disagrees with our hard-coded autoprefixer config?Lastly, I couldn't find anything on supporting PostCSS within CRA 2.0
I'm certain the answer should be: "leave it to them to figure out in full control mode", but should we do anything further for the rare situation where CRA users eject and come up with their own PostCSS config? It should be simple enough, and it further justifies the need to modularize our increasingly complex style-related webpack config logic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just trying to stick to what the CRA config had.
postcss-preset-envwill look at thebrowserlistset in thepackage.json, so not sure where this differs from what was there before.I agree the styles-related webpack config needs to be modularized. To be honest I picked up this PR as I thought it was going to be simple and would allow me to get my toes wet with the codebase. It's turning out to be a bit bigger of a change haha. The more I add the more I'm not sure if I'm conflicting with some of the other apps.
Wouldn't #4411 support PostCSS within CRA 2.0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right! It would, as long as I do a simple conditional tacked onto your
.modulescheck.I think you should still work without worry. Keep in mind that this
.modulesparadigm isn't common practice and seems very specific to CRA 2.0It's conditional logic that precludes CRA 2.0 specifically. You wont infect other consumers at this point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're nearly there - apologies for nitpicking - this is good work!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, nitpicking is fine, I'm just trying to follow along lol.
It looks like the post-css is clashing with the
markopackage. Both mine and yours are failing that build. I'm not familiar with themarkoframework so I'm not 100% sure but it looks like it's because we removed thepostcss: {}line from the default config. https://github.com/storybooks/storybook/blob/1f127f0b1d0cdd71708ed4864bc79055b1796f4f/lib/core/src/server/config/webpack.config.default.js#L24I added that back in and now the
markopackage seems to work.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice usage of the new suggestion feature 😉
I haven't dived in at all, but I'm certain we can provide an escape hatch for one consumer if we're providing a green path for another.
This could turn into a slippery slope, but we are trying to support one of the biggest consumer bases via this PR.
@ndelangen - know any Marko experts? I haven't checked to see what the issue is, but just figured I'd ask ahead of time.