Support css-modules #4405

chadfawcett · 2018-10-13T04:41:29Z

Issue: Resolve #4306

What I did

Created a react webpack preset that pulls style rules from create-react-app's congis.

Support for css modules (*.module.css)
Support for sass (scss and sass)
Support for sass modules (*.module.scss and *.module.sass)

~~> The default Webpack config of Storybook is balanced for a medium-size project (specially created with Create React App) or a library.~~

~~I know @ndelangen mentioned that this statement is a little outdated, but I feel this change should be okay as it shouldn't cause any harm adding css modules as is.~~

@igor-dv I chose to propose the simpler option first, as presets aren't well defined yet, and I saw @ndelangen was working on separating the manager and preview webpack processes. Either of these may result in a better implementation of the CRA webpack config in the future.

~~I can work on implementing the presets approach if this method is not sufficient. What are your thoughts?~~

How to test

You can update examples/cra-kitchen-sink to the following in order to test (example code taken from initial reporting issue):

//  App.module.css (New file)
.text {
  color: red;
  font-size: 2rem;
}

//  App.js (Existing file)
...
import styles from './App.module.css'

const App = () => (
  ...
    <h2 className={styles.text}>Welcome to React</h2>
  ...
)

Then run yarn storybook from the examples/cra-kitchen-sink repo and the "Welcome to React" will be red.

Does this need a new example in the kitchen sink apps?
Maybe? I can add something similar to the code above if we want it committed into the repo.

…ules

codecov · 2018-10-13T05:10:53Z

Codecov Report

Merging #4405 into master will decrease coverage by 0.21%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #4405      +/-   ##
=========================================
- Coverage   35.91%   35.7%   -0.22%     
=========================================
  Files         555     557       +2     
  Lines        6668    6708      +40     
  Branches      871     879       +8     
=========================================
  Hits         2395    2395              
- Misses       3826    3858      +32     
- Partials      447     455       +8

Impacted Files	Coverage Δ
app/react/src/server/options.js	`0% <ø> (ø)`	⬆️
lib/core/src/server/core-preset-webpack-custom.js	`0% <0%> (ø)`	⬆️
app/react/src/server/cra_config.js	`0% <0%> (ø)`
lib/core/src/server/config.js	`0% <0%> (ø)`	⬆️
...pp/react/src/server/framework-preset-cra-styles.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 db8be4b...6297d48. Read the comment docs.

…ules

ndelangen · 2018-10-13T08:26:46Z

Awesome work @chadfawcett 🎈

gabrielcsapo · 2018-10-13T08:26:48Z

@chadfawcett love the change! Would this be better isolated in the examples rather than supported at the core level?

chadfawcett · 2018-10-13T08:30:41Z

@gabrielcsapo if it was isolated at the example level, wouldn’t that mean it was an end user override similar to the workaround proposed in the original issue?

ndelangen · 2018-10-13T08:34:48Z

@chadfawcett is correct here.

I think the best approach would be to narrow this into a preset specific for either React of CRA.

I'm sure @igor-dv will be able to help with this.

chadfawcett · 2018-10-13T08:55:56Z

I’ve got a fairly good idea of how to implement the preset, but it’s a lot more work to merge the default and cra configs. So I thought I’d propose this solution first 😜

ndelangen · 2018-10-13T15:29:51Z

I fear this will not work well with other frameworks.

Nevermind

chadfawcett · 2018-10-13T18:07:17Z

@ndelangen Would it cause anymore issues with other frameworks than the existing css rules?

igor-dv

I think we need to be very careful with the changes like this since you are adding it to the default webpack config and it may really break other usages/frameworks without the real benefit to them. Also, the convention of file.module.css - I am not sure how much this is common (for example I don't use it =). So, who is really winning from this change?

I’ve got a fairly good idea of how to implement the preset, but it’s a lot more work to merge the default and cra configs

Integration with CRA is acutaly the thing we need to do 🤷‍♂️ . Also, I assume we do not need to merge the full config, but only styling, at least for now IMO (I didn't see any other issues that are not related to styling).

P.S sorry for being unresponsive, I am returning from the vacation tomorrow =)

chadfawcett · 2018-10-13T20:38:33Z

@igor-dv Okay so it seems the consensus is that updating the default config directly is bad, and we should use a preset. There are two approaches we could take:

Detect cra and automatically merge in parts of the cra webpack config (I could use your feedback on my proposed method of detection)
Apply css module support to all @storybook/react. This would just be a css modules preset that we add to storybook/app/react/src/server/options.js

For reference, I figured #4298 could also be fixed if we merged more than just the style rules.

Thoughts?

igor-dv · 2018-10-13T20:44:42Z

I vote for 1 =) Anything neede to be merged should be merged

ndelangen · 2018-10-13T23:08:40Z

Would be cool if we'd load the actual webpack config CRA uses, and merge the relevant bits

gabrielcsapo · 2018-10-13T23:29:17Z

I agree 1 is the way to go

Hypnosphi · 2018-10-14T00:40:19Z

I fear this will not work well with other frameworks.

Why? CSS Modules are completely framework agnostic and has no direct connection with React

ndelangen · 2018-10-14T10:26:34Z

@Hypnosphi you're 100% correct as always.
I guess if we test for /.module.(css|scss|sass)$/ this should be fine.
That's very unlikely to clash with rules already set by the user?

But long term we want to move all these things into presets right?

chadfawcett · 2018-10-14T19:07:27Z

@Hypnosphi @ndelangen That's what I thought. It may not be the best implementation, but it's and easy win that shouldn't cause issue with anything else.

Should I update the regex to support sass/scss and then call this PR done? Leave the CRA preset to a later PR?

kylemh

😍

kylemh · 2018-10-20T06:43:47Z

app/react/package.json

-    "react-dev-utils": "^6.0.5"
+    "react-dev-utils": "^6.0.5",
+    "semver": "^5.6.0",
+    "webpack": "^4.21.0"


🤔 I wonder if we could make Webpack 4+ a peer dep instead?

webpack is a direct dep in a bunch of the SB packages (including core). It will be complicated to make it a peer (like for supporting different major version of webpack)

CC #4371, #4430

Edit: this is not a CR comment, just a reply to @kylemh =)

kylemh · 2018-10-20T06:48:00Z

app/react/src/server/cra_config.js

+  const plugins = [...baseConfig.plugins];
+  if (baseConfig.mode === 'production')
+    plugins.push(
+      new MiniCssExtractPlugin({


Does this exclude interop for CRAv1? I don't really care about that, but it could be something to consider.

kylemh · 2018-10-20T06:51:41Z

app/react/src/server/cra_config.js

+  }
+}
+
+export function getStyleRules(rules) {


I'd love to get some comments throughout this function. Not certain what's occurring.

It looks like a way to concatenate across the subset of style loading possibilities given the array of file suffixes we're sifting?

kylemh · 2018-10-20T06:54:59Z

app/react/src/server/cra_config.js

+
+export function applyCRAWebpackConfig(baseConfig) {
+  // Remove style rules from baseConfig
+  const baseRulesExcludingStyles = baseConfig.module.rules.filter(


Very nice 👏

kylemh · 2018-10-20T06:56:01Z

app/react/src/server/cra_config.js

+  const craWebpackConfig = getCraWebpackConfig(baseConfig.mode);
+
+  //  Concat will ensure rules is an array
+  const craStyleRules = getStyleRules([].concat(craWebpackConfig.module.rules));


could we not simply do:

[ ...craWebpackConfig.module.rules ]

Interested to hear more on "ensur[ing] rules is an array"

Whoops. I was thinking module.rules could be something other than an array. This is the case for rule.test (array, regex, function, string, etc) but not the case for module.rules. I've removed the concat statement.

kylemh · 2018-10-20T06:57:19Z

app/react/src/server/cra_config.js

+  const plugins = [...baseConfig.plugins];
+  if (baseConfig.mode === 'production')
+    plugins.push(
+      new MiniCssExtractPlugin({


FWIW we dont really need autoprefixer in the default ruleset either 🤷‍♂️

I'm a fan of good defaults 👍

kylemh · 2018-10-20T07:02:10Z

(This comment isn't about this PR, but a general point of discussion)
This preset approach is very interesting.

I wonder if we could set ground rules and documentation about adding new ones.

Most importantly, I wonder if we could give the user control in using presets thus removing the need for us to handle edge-cases. Presets can only be selected under X assumptions - otherwise, you're on your own.

igor-dv

Now I am happy 👍

igor-dv · 2018-10-21T09:02:16Z

app/react/package.json

-    "react-dev-utils": "^6.0.5"
+    "react-dev-utils": "^6.0.5",
+    "semver": "^5.6.0",
+    "webpack": "^4.21.0"


webpack is a direct dep in a bunch of the SB packages (including core). It will be complicated to make it a peer (like for supporting different major version of webpack)

CC #4371, #4430

Edit: this is not a CR comment, just a reply to @kylemh =)

igor-dv · 2018-10-21T09:07:21Z

app/react/src/server/cra_config.js

+    // eslint-disable-next-line global-require, import/no-extraneous-dependencies, prefer-destructuring
+    normalizeCondition = require('webpack/lib/RuleSet').normalizeCondition;
+    // eslint-disable-next-line global-require, import/no-extraneous-dependencies
+    MiniCssExtractPlugin = require('mini-css-extract-plugin');


So you can require it already in the applyCRAWebpackConfig func and remove the state from the module (IMO Node caches imported modules, so you don't need to care about this issue).

igor-dv · 2018-10-21T09:21:45Z

@kylemh

I wonder if we could set ground rules and documentation about adding new ones.

We should, it's on my todo, at least for the maintainers,

Most importantly, I wonder if we could give the user control in using presets thus removing the need for us to handle edge-cases. Presets can only be selected under X assumptions - otherwise, you're on your own.

Custom presets could be used by the users, but we didn't want to open this feature until #4169 (and because we kinda need to tests this 😅 )

Regarding the "selective" presets thingy, how can the API for this look like?

igor-dv · 2018-10-21T09:42:15Z

Almost forgot. Do we have any CLI tests testing this setup?

shilman · 2018-10-22T07:29:42Z

@chadfawcett @igor-dv @ndelangen would love to get this into an RC -- any chance we can merge this today?

igor-dv · 2018-10-22T13:22:02Z

The CLI tests could be added after

chadfawcett · 2018-10-22T16:57:06Z

Sorry I was in the middle of responding to comments when GitHub started having issues yesterday. Glad my latest push synced in time.

@igor-dv As for the CLI tests, all of the fixtures have React 15 as dependencies. This brought to light that I needed to test that react-scripts was at least v2, but that meant the CLI tests were no longer testing this new preset.

Should the fixtures be updated to React 16? Or should additional fixtures be created?

igor-dv · 2018-10-23T15:10:04Z

I think React 15 is not supported now, I am not sure how it works. But yeah, you can just create a new one.

igor-dv · 2018-10-23T15:18:29Z

app/react/src/server/cra_config.js

+  const plugins = [...baseConfig.plugins];
+  if (baseConfig.mode === 'production') {
+    // eslint-disable-next-line global-require, import/no-extraneous-dependencies
+    const MiniCssExtractPlugin = require('mini-css-extract-plugin');


On a second thought, I think this can make problems if 'mini-css-extract-plugin' is not installed as the root package in node_modules 🤔

Maybe we need to do something like this instead ?

const miniCssExtractPlugin = craWebpackConfig.plugins.find(plugin => plugin.constructor.name === 'MiniCssExtractPlugin');

I think you were right initially. Since we check to make sure react-scripts is installed before calling applyCRAWebpackConfig (I Just noticed this has all caps for CRA), then we know mini-css-extract-plugin is present.

If we want to use it, we have to depend on it

we know mini-css-extract-plugin is present.

There's no guarantee that it's available in root node_modules not in node_modules/react-scripts/node_modules. You can never rely on npm hoisting

Looks like #4524 is broken now because of this

Created #4534 to hopefully resolve this

Hypnosphi · 2018-10-23T23:49:46Z

app/react/package.json

@@ -33,12 +33,15 @@
    "@babel/runtime": "^7.1.2",
    "@emotion/styled": "^0.10.6",
    "@storybook/core": "4.0.0-rc.1",
+    "@storybook/node-logger": "^3.4.11",


It should be "4.0.0-rc.3"

chadfawcett added 2 commits October 10, 2018 22:42

Support css-modules

7594e87

Merge branch 'master' of github.com:storybooks/storybook into css-mod…

3ccf2bb

…ules

chadfawcett requested review from alterx, igor-dv and tmeasday as code owners October 13, 2018 04:41

Merge branch 'master' of github.com:storybooks/storybook into css-mod…

535d2b6

…ules

gabrielcsapo added the feature request label Oct 13, 2018

ndelangen assigned chadfawcett Oct 13, 2018

ndelangen added compatibility with other tools react labels Oct 13, 2018

ndelangen added this to the next milestone Oct 13, 2018

pksunkara previously approved these changes Oct 13, 2018

View reviewed changes

igor-dv requested changes Oct 13, 2018

View reviewed changes

ndelangen approved these changes Oct 14, 2018

View reviewed changes

Add webpack as dependency to app/react

6297d48

kylemh approved these changes Oct 20, 2018

View reviewed changes

igor-dv approved these changes Oct 21, 2018

View reviewed changes

chadfawcett added 3 commits October 21, 2018 15:41

Remove module state for imports

5f00d84

Describe getStyleRules better

e2cf993

Remove unnecessary concat

f42fb81

igor-dv merged commit d704510 into storybookjs:master Oct 22, 2018

chadfawcett deleted the css-modules branch October 22, 2018 16:49

igor-dv reviewed Oct 23, 2018

View reviewed changes

Hypnosphi mentioned this pull request Oct 23, 2018

Add angular@7 cli tests #4524

Merged

chadfawcett mentioned this pull request Oct 23, 2018

Add mini-css-extract-plugin to app/react dependencies #4534

Merged

Hypnosphi reviewed Oct 23, 2018

View reviewed changes

igor-dv mentioned this pull request Oct 24, 2018

Fix the node-logger version in app/react #4539

Merged

tirli mentioned this pull request Dec 6, 2018

SCSS doesn't work out of the box on Windows #4940

Closed

Support css-modules #4405

Support css-modules #4405

Conversation

chadfawcett commented Oct 13, 2018 • edited

What I did

How to test

codecov bot commented Oct 13, 2018 • edited

Codecov Report

ndelangen commented Oct 13, 2018

gabrielcsapo commented Oct 13, 2018

chadfawcett commented Oct 13, 2018

ndelangen commented Oct 13, 2018

chadfawcett commented Oct 13, 2018

ndelangen commented Oct 13, 2018

chadfawcett commented Oct 13, 2018

igor-dv left a comment

Choose a reason for hiding this comment

chadfawcett commented Oct 13, 2018

igor-dv commented Oct 13, 2018

ndelangen commented Oct 13, 2018

gabrielcsapo commented Oct 13, 2018

Hypnosphi commented Oct 14, 2018 • edited

ndelangen commented Oct 14, 2018

chadfawcett commented Oct 14, 2018

kylemh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-dv Oct 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kylemh commented Oct 20, 2018 • edited

igor-dv left a comment

Choose a reason for hiding this comment

igor-dv Oct 21, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

igor-dv commented Oct 21, 2018

igor-dv commented Oct 21, 2018

shilman commented Oct 22, 2018

igor-dv commented Oct 22, 2018

chadfawcett commented Oct 22, 2018

igor-dv commented Oct 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi Oct 23, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chadfawcett Oct 23, 2018 • edited

Choose a reason for hiding this comment

Hypnosphi Oct 23, 2018 • edited

Choose a reason for hiding this comment

chadfawcett commented Oct 13, 2018 •

edited

codecov bot commented Oct 13, 2018 •

edited

Hypnosphi commented Oct 14, 2018 •

edited

igor-dv Oct 21, 2018 •

edited

kylemh commented Oct 20, 2018 •

edited

igor-dv Oct 21, 2018 •

edited

Hypnosphi Oct 23, 2018 •

edited

chadfawcett Oct 23, 2018 •

edited

Hypnosphi Oct 23, 2018 •

edited