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 css-modules #4405

Merged
merged 27 commits into from
Oct 22, 2018
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
7594e87
Support css-modules
chadfawcett Oct 11, 2018
3ccf2bb
Merge branch 'master' of github.com:storybooks/storybook into css-mod…
chadfawcett Oct 11, 2018
535d2b6
Merge branch 'master' of github.com:storybooks/storybook into css-mod…
chadfawcett Oct 13, 2018
0441fc5
Add [module.]scss/sass loader support
chadfawcett Oct 16, 2018
fefd86a
Update yarn.lock
chadfawcett Oct 16, 2018
4a6fd1b
Merge branch 'master' of github.com:storybooks/storybook into css-mod…
chadfawcett Oct 17, 2018
f40b98c
Add specific eslint ignore reasoning and fix default postcss
chadfawcett Oct 17, 2018
b12ffae
Refactor webpack style rules to separate file
chadfawcett Oct 17, 2018
3f88f74
Add sass-loader as core dependency
chadfawcett Oct 17, 2018
da0ad40
Update yarn.lock with bootstrap
chadfawcett Oct 17, 2018
76a668f
Update angular-cli style filter
chadfawcett Oct 17, 2018
39e2c3e
Merge branch 'master' of github.com:storybooks/storybook into css-mod…
chadfawcett Oct 17, 2018
980f394
Revert default webpack config changes
chadfawcett Oct 18, 2018
9719566
Merge branch 'master' of github.com:storybooks/storybook into framewo…
chadfawcett Oct 18, 2018
53004c4
CRA webpack style preset
chadfawcett Oct 18, 2018
41826b6
Add suport for production config
chadfawcett Oct 19, 2018
1753127
Update autoprefixer from revert
chadfawcett Oct 19, 2018
e1b73c7
Check for react-scripts before calling merge
chadfawcett Oct 19, 2018
5949b40
Update yarn.lock
chadfawcett Oct 19, 2018
b2f9e66
Only load react-scripts config newer than v2
chadfawcett Oct 19, 2018
950e2a6
Merge remote-tracking branch 'origin/master' into css-modules
igor-dv Oct 19, 2018
2873911
Move `webpackFinal` so custom webpack config could still edit things :-/
igor-dv Oct 19, 2018
cf34baa
Extract getCraWebpackConfig
chadfawcett Oct 19, 2018
6297d48
Add webpack as dependency to app/react
chadfawcett Oct 19, 2018
5f00d84
Remove module state for imports
chadfawcett Oct 21, 2018
e2cf993
Describe getStyleRules better
chadfawcett Oct 21, 2018
f42fb81
Remove unnecessary concat
chadfawcett Oct 21, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/react/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Copy link
Member

@Hypnosphi Hypnosphi Oct 23, 2018

Choose a reason for hiding this comment

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

It should be "4.0.0-rc.3"

"babel-plugin-react-docgen": "^2.0.0",
"common-tags": "^1.8.0",
"global": "^4.3.2",
"lodash": "^4.17.11",
"prop-types": "^15.6.2",
"react-dev-utils": "^6.0.5"
"react-dev-utils": "^6.0.5",
"semver": "^5.6.0",
"webpack": "^4.21.0"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

@igor-dv igor-dv Oct 21, 2018

Choose a reason for hiding this comment

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

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 =)

},
"peerDependencies": {
"babel-loader": "^7.0.0 || ^8.0.0",
Expand Down
84 changes: 84 additions & 0 deletions app/react/src/server/cra_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
import semver from 'semver';
import { normalizeCondition } from 'webpack/lib/RuleSet';

let MiniCssExtractPlugin;

export function isReactScriptsInstalled() {
try {
// eslint-disable-next-line global-require, import/no-extraneous-dependencies
const reactScriptsJson = require('react-scripts/package.json');
if (semver.lt(reactScriptsJson.version, '2.0.0')) return false;

// eslint-disable-next-line global-require, import/no-extraneous-dependencies
MiniCssExtractPlugin = require('mini-css-extract-plugin');
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Do we need it as a dep?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't need this as a dependency since we're first checking if react-scripts is installed. If so, then mini-css-extract-plugin will be installed.

Copy link
Member

Choose a reason for hiding this comment

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

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).

return true;
} catch (e) {
return false;
}
}

export function getStyleRules(rules) {
Copy link
Member

Choose a reason for hiding this comment

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

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?

// Extensions of style rules we're interested in
const extensions = ['.css', '.scss', '.sass', '.module.css', '.module.scss', '.module.sass'];

return rules.reduce((styleRules, rule) => {
if (rule.test && extensions.some(normalizeCondition(rule.test))) {
// If the base test is for styles, return early
return styleRules.concat(rule);
}

if (!rule.test && rule.oneOf) {
styleRules.push(...getStyleRules(rule.oneOf));
}

if (!rule.test && rule.rules) {
styleRules.push(...getStyleRules(rule.rules));
}

return styleRules;
}, []);
}

export function getCraWebpackConfig(mode) {
if (mode === 'production') {
// eslint-disable-next-line global-require, import/no-extraneous-dependencies
return require('react-scripts/config/webpack.config.prod');
}

// eslint-disable-next-line global-require, import/no-extraneous-dependencies
return require('react-scripts/config/webpack.config.dev');
}

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

Choose a reason for hiding this comment

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

Very nice 👏

rule => !rule.test || !['.css', '.scss', '.sass'].some(normalizeCondition(rule.test))
);

// Load create-react-app config
const craWebpackConfig = getCraWebpackConfig(baseConfig.mode);

// Concat will ensure rules is an array
const craStyleRules = getStyleRules([].concat(craWebpackConfig.module.rules));
Copy link
Member

Choose a reason for hiding this comment

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

could we not simply do:

[ ...craWebpackConfig.module.rules ]

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.


// Add css minification for production
const plugins = [...baseConfig.plugins];
if (baseConfig.mode === 'production')
plugins.push(
new MiniCssExtractPlugin({
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need this, though?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope! I just thought minification would be good for production.

Also, if we don't want to include this plugin, then we have to filter out the related rule from all of the style rules. Adding the plugin was less work than removing it.
https://github.com/facebook/create-react-app/blob/master/packages/react-scripts/config/webpack.config.prod.js#L65-L71

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

I'm a fan of good defaults 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@kylemh This preset checks for react-scripts greater than v2. So it would not do anything for CRAv1.

// Options similar to the same options in webpackOptions.output
// both options are optional
filename: 'static/css/[name].[contenthash:8].css',
chunkFilename: 'static/css/[name].[contenthash:8].chunk.css',
})
);

return {
...baseConfig,
module: {
...baseConfig.module,
rules: [...baseRulesExcludingStyles, ...craStyleRules],
},
plugins,
};
}
13 changes: 13 additions & 0 deletions app/react/src/server/framework-preset-cra-styles.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { logger } from '@storybook/node-logger';
import { applyCRAWebpackConfig, isReactScriptsInstalled } from './cra_config';

export function webpackFinal(config) {
igor-dv marked this conversation as resolved.
Show resolved Hide resolved
if (!isReactScriptsInstalled()) {
logger.info('=> Using base config because react-scripts is not installed.');
return config;
}

logger.info('=> Loading create-react-app config.');

return applyCRAWebpackConfig(config);
}
1 change: 1 addition & 0 deletions app/react/src/server/options.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,5 +6,6 @@ export default {
frameworkPresets: [
require.resolve('./framework-preset-react.js'),
require.resolve('./framework-preset-react-docgen.js'),
require.resolve('./framework-preset-cra-styles.js'),
],
};
5 changes: 1 addition & 4 deletions lib/core/src/server/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ function wrapCorePresets(presets) {
return {
babel: async (config, args) => presets.apply('babel', config, args),
webpack: async (config, args) => presets.apply('webpack', config, args),
webpackFinal: async (config, args) => presets.apply('webpackFinal', config, args),
preview: async (config, args) => presets.apply('preview', config, args),
manager: async (config, args) => presets.apply('manager', config, args),
};
Expand Down Expand Up @@ -35,9 +34,7 @@ async function getWebpackConfig(options, presets) {
manager: await presets.manager([], options),
};

const webpackConfig = await presets.webpack({}, { ...options, babelOptions, entries });

return presets.webpackFinal(webpackConfig, options);
return presets.webpack({}, { ...options, babelOptions, entries });
}

export default async options => {
Expand Down
23 changes: 19 additions & 4 deletions lib/core/src/server/core-preset-webpack-custom.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,39 @@ function informAboutCustomConfig(defaultConfigName) {
logger.info(`=> Using default webpack setup based on "${defaultConfigName}".`);
}

export function webpack(config, { configDir, configType, defaultConfigName }) {
function wrapPresets(presets) {
return {
webpackFinal: async (config, args) => presets.apply('webpackFinal', config, args),
};
}

async function createFinalDefaultConfig(presets, config, options) {
const defaultConfig = createDefaultWebpackConfig(config);
return presets.webpackFinal(defaultConfig, options);
Copy link
Member

Choose a reason for hiding this comment

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

webpackFinal will be called twice. One for the defaultConfig and second for the baseConfig. That is how it worked before the presets feature 🤷‍♂️

}

export async function webpack(config, options) {
const { configDir, configType, defaultConfigName } = options;
const presets = wrapPresets(options.presets);

const finalConfig = await presets.webpackFinal(config, options);

// Check whether user has a custom webpack config file and
// return the (extended) base configuration if it's not available.
const customConfig = loadCustomWebpackConfig(configDir);

if (customConfig === null) {
informAboutCustomConfig(defaultConfigName);
return defaultConfig;
return createFinalDefaultConfig(presets, config, options);
}

if (typeof customConfig === 'function') {
logger.info('=> Loading custom webpack config (full-control mode).');
return customConfig(config, configType, defaultConfig);
const finalDefaultConfig = await createFinalDefaultConfig(presets, config, options);
return customConfig(finalConfig, configType, finalDefaultConfig);
}

logger.info('=> Loading custom webpack config (extending mode).');

return mergeConfigs(config, customConfig);
return mergeConfigs(finalConfig, customConfig);
}
11 changes: 9 additions & 2 deletions yarn.lock
Original file line number Diff line number Diff line change
Expand Up @@ -1880,6 +1880,13 @@
"@storybook/react-simple-di" "^1.2.1"
babel-runtime "6.x.x"

"@storybook/node-logger@^3.4.11":
version "3.4.11"
resolved "https://registry.yarnpkg.com/@storybook/node-logger/-/node-logger-3.4.11.tgz#a6684a4c21f74dae937cd9f202deec0932d3f3b5"
integrity sha512-eCjvZsCwZTcjDOeG7JDEVs5bugyybpAFu/4+X3hfikxGBBjnx2NtjJIfIsriUKa1O559+aFGUG73wogYAjudhg==
dependencies:
npmlog "^4.1.2"

"@storybook/podda@^1.2.3":
version "1.2.3"
resolved "https://registry.yarnpkg.com/@storybook/podda/-/podda-1.2.3.tgz#53c4a1a3f8c7bbd5755dff5c34576fd1af9d38ba"
Expand Down Expand Up @@ -5753,12 +5760,12 @@ caniuse-api@^3.0.0:
lodash.memoize "^4.1.2"
lodash.uniq "^4.5.0"

caniuse-lite@^1.0.0, caniuse-lite@^1.0.30000844, caniuse-lite@^1.0.30000864, caniuse-lite@^1.0.30000884, caniuse-lite@^1.0.30000887, caniuse-lite@^1.0.30000889, caniuse-lite@^1.0.30000890:
caniuse-lite@^1.0.0, caniuse-lite@^1.0.30000844, caniuse-lite@^1.0.30000864, caniuse-lite@^1.0.30000884, caniuse-lite@^1.0.30000887, caniuse-lite@^1.0.30000889:
version "1.0.30000890"
resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30000890.tgz#86a18ffcc65d79ec6a437e985761b8bf1c4efeaf"
integrity sha512-4NI3s4Y6ROm+SgZN5sLUG4k7nVWQnedis3c/RWkynV5G6cHSY7+a8fwFyn2yoBDE3E6VswhTNNwR3PvzGqlTkg==

caniuse-lite@^1.0.30000892:
caniuse-lite@^1.0.30000890, caniuse-lite@^1.0.30000892:
version "1.0.30000892"
resolved "https://registry.yarnpkg.com/caniuse-lite/-/caniuse-lite-1.0.30000892.tgz#344d2b51ee3ff5977537da4aa449c90eec40b759"
integrity sha512-X9rxMaWZNbJB5qjkDqPtNv/yfViTeUL6ILk0QJNxLV3OhKC5Acn5vxsuUvllR6B48mog8lmS+whwHq/QIYSL9w==
Expand Down