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 all 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
85 changes: 85 additions & 0 deletions app/react/src/server/cra_config.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
import semver from 'semver';
import { normalizeCondition } from 'webpack/lib/RuleSet';

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;
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 at least one style extension satisfies the rule test, the rule is one
// we want to extract
if (rule.test && extensions.some(normalizeCondition(rule.test))) {
// If the base test is for styles, return early
return styleRules.concat(rule);
}

// Get any style rules contained in rule.oneOf
if (!rule.test && rule.oneOf) {
styleRules.push(...getStyleRules(rule.oneOf));
}

// Get any style rules contained in rule.rules
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 any rules from baseConfig that test true for any one of the extensions
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);

const craStyleRules = getStyleRules(craWebpackConfig.module.rules);

// Add css minification for production
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');
Copy link
Member

Choose a reason for hiding this comment

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

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 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we need to do something like this instead ?

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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

Copy link
Member

Choose a reason for hiding this comment

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

Looks like #4524 is broken now because of this

Copy link
Member Author

@chadfawcett chadfawcett Oct 23, 2018

Choose a reason for hiding this comment

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

Created #4534 to hopefully resolve this

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