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

Add support for CSS Modules with explicit filename - [name].module.css #2285

Merged
merged 5 commits into from
Jan 17, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
44 changes: 35 additions & 9 deletions packages/react-scripts/config/webpack.config.dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,20 @@ const publicUrl = '';
// Get environment variables to inject into our app.
const env = getClientEnvironment(publicUrl);

// Options for PostCSS as we reference these options twice
// Adds vendor prefixing to support IE9 and above
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment outdated (since we don't use a particular config anymore)

const postCSSLoaderOptions = {
// Necessary for external CSS imports to work
// https://github.com/facebookincubator/create-react-app/issues/2677
ident: 'postcss',
plugins: () => [
require('postcss-flexbugs-fixes'),
autoprefixer({
flexbox: 'no-2009',
}),
],
};

// This is the development configuration.
// It is focused on developer experience and fast rebuilds.
// The production configuration is different and lives in a separate file.
Expand Down Expand Up @@ -209,8 +223,10 @@ module.exports = {
// "style" loader turns CSS into JS modules that inject <style> tags.
// In production, we use a plugin to extract that CSS to a file, but
// in development "style" loader enables hot editing of CSS.
// By default we support CSS Modules with the extension .module.css
{
test: /\.css$/,
exclude: /\.module\.css$/,
use: [
require.resolve('style-loader'),
{
Expand All @@ -221,18 +237,28 @@ module.exports = {
},
{
loader: require.resolve('postcss-loader'),
options: postCSSLoaderOptions,
},
],
},
// Adds support for CSS Modules (https://github.com/css-modules/css-modules)
// using the extension .module.css
{
test: /\.module\.css$/,
use: [
require.resolve('style-loader'),
{
loader: require.resolve('css-loader'),
options: {
// Necessary for external CSS imports to work
// https://github.com/facebookincubator/create-react-app/issues/2677
ident: 'postcss',
plugins: () => [
require('postcss-flexbugs-fixes'),
autoprefixer({
flexbox: 'no-2009',
}),
],
importLoaders: 1,
modules: true,
localIdentName: '[path]__[name]___[local]',
},
},
{
loader: require.resolve('postcss-loader'),
options: postCSSLoaderOptions,
},
],
},
// "file" loader makes sure those assets get served by WebpackDevServer.
Expand Down
59 changes: 50 additions & 9 deletions packages/react-scripts/config/webpack.config.prod.js
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,20 @@ const extractTextPluginOptions = shouldUseRelativeAssetPaths
{ publicPath: Array(cssFilename.split('/').length).join('../') }
: {};

// Options for PostCSS as we reference these options twice
// Adds vendor prefixing to support IE9 and above
const postCSSLoaderOptions = {

Choose a reason for hiding this comment

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

A config file might be better since we are copy pasting this on the production and on the dev version.

IMO it would be even better to merge both webpack config files and simply use variables to determine the correct environment. It would also help a lot when you're ejecting it and add/change/remove things but this is just IMHO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was copy and pasted previously as well. For now I think leaving it rather than making another config file is best and it simplifies the code base (at cost of repetition).

The issue should be addressed in #2108 when the target browsers is pulled out into the package.json config.

Thanks for the feedback. Happy to discuss further.

// Necessary for external CSS imports to work
// https://github.com/facebookincubator/create-react-app/issues/2677
ident: 'postcss',
plugins: () => [
require('postcss-flexbugs-fixes'),
autoprefixer({
flexbox: 'no-2009',
}),
],
};

// This is the production configuration.
// It compiles slowly and is focused on producing a fast and minimal bundle.
// The development configuration is different and lives in a separate file.
Expand Down Expand Up @@ -221,8 +235,10 @@ module.exports = {
// tags. If you use code splitting, however, any async bundles will still
// use the "style" loader inside the async code so CSS from them won't be
// in the main CSS file.
// By default we support CSS Modules with the extension .module.css
{
test: /\.css$/,
exclude: /\.module\.css$/,
loader: ExtractTextPlugin.extract(
Object.assign(
{
Expand All @@ -243,18 +259,43 @@ module.exports = {
},
{
loader: require.resolve('postcss-loader'),
options: postCSSLoaderOptions,
},
],
},
extractTextPluginOptions
)
),
// Note: this won't work without `new ExtractTextPlugin()` in `plugins`.
},
// Adds support for CSS Modules (https://github.com/css-modules/css-modules)
// using the extension .module.css
{
test: /\.module\.css$/,
loader: ExtractTextPlugin.extract(
Object.assign(
{
fallback: {
loader: require.resolve('style-loader'),
options: {
hmr: false,
},
},
use: [
{
loader: require.resolve('css-loader'),
options: {
// Necessary for external CSS imports to work
// https://github.com/facebookincubator/create-react-app/issues/2677
ident: 'postcss',
plugins: () => [
require('postcss-flexbugs-fixes'),
autoprefixer({
flexbox: 'no-2009',
}),
],
importLoaders: 1,
minimize: true,
sourceMap: shouldUseSourceMap,
modules: true,
localIdentName: '[path]__[name]___[local]',
Copy link

Choose a reason for hiding this comment

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

Should the production build expose the app architecture?
Maybe use this property only when shouldUseSourceMap is true

Copy link

Choose a reason for hiding this comment

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

Personally I always go for '[name]-[local]'. It's very readable and also deterministic, therefore it works well with e2e tests and error reporting tools. The only constraint is that you shouldn't have components with identical names, which for me is something I do anyway. Maybe other people have a different opinion here though as this is can be potential error source.

In regards to @klzns comment, I'd also appreciate not exposing the file path – at least not when shouldUseSourceMap is false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why does it need to be deterministic and targetable? kind of kills the whole module thing. Why not recommend people to add additional static class if it needs to be targetable? Im upp for hashes.

Copy link

@sompylasar sompylasar Jan 17, 2018

Choose a reason for hiding this comment

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

Why does it need to be deterministic and targetable? kind of kills the whole module thing. Why not recommend people to add additional static class if it needs to be targetable? Im upp for hashes.

Web marketing people often freak out if they cannot attach the out-of-the-box tools like HeapAnalytics or MouseFlow to specific elements of a website/webapp by id/classname (or if the classnames they attach to are changing with each release).

Copy link

Choose a reason for hiding this comment

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

That's another example. Using a hash is be fine by me, and the mentioned use cases can still be addressed with static classes or e.g. data attributes. It's just a personal preference I thought I'll mention in case somebody else finds this interesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

The only constraint is that you shouldn't have components with identical names, which for me is something I do anyway.

This sounds like a very artificial constraint. I expect that people will trip over this if it's not enforced.

Copy link

@lnhrdt lnhrdt Feb 3, 2018

Choose a reason for hiding this comment

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

Why not recommend people to add additional static class if it needs to be targetable?

Great point @andriijas. I agree that we should recommend people decouple styling classes from identifiers for testing or the tools like the ones @sompylasar mentioned. The fact that we used styling classes for so many years as hooks for these tools wasn't because they were actually related concerns, it was just all we had.

Make it easy to do the right thing / hard to do the wrong thing.

},
},
{
loader: require.resolve('postcss-loader'),
options: postCSSLoaderOptions,
},
],
},
extractTextPluginOptions
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,16 @@ describe('Integration', () => {
).to.match(/#feature-css-inclusion\{background:.+;color:.+}/);
});

it('css modules inclusion', async () => {
const doc = await initDOM('css-modules-inclusion');

expect(
doc.getElementsByTagName('style')[0].textContent.replace(/\s/g, '')
).to.match(
/.+__style-module___cssModulesInclusion+\{background:.+;color:.+}/
);
});

it('image inclusion', async () => {
const doc = await initDOM('image-inclusion');

Expand Down
5 changes: 5 additions & 0 deletions packages/react-scripts/fixtures/kitchensink/src/App.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,11 @@ class App extends Component {
this.setFeature(f.default)
);
break;
case 'css-modules-inclusion':
import(
'./features/webpack/CssModulesInclusion'
).then(f => this.setFeature(f.default));
break;
case 'custom-interpolation':
import('./features/syntax/CustomInterpolation').then(f =>
this.setFeature(f.default)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React from 'react';
import styles from './assets/style.module.css';

export default () => (
<p className={styles.cssModulesInclusion}>CSS Modules are working!</p>
);
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
/**
* Copyright (c) 2015-present, Facebook, Inc.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/

import React from 'react';
import ReactDOM from 'react-dom';
import CssModulesInclusion from './CssModulesInclusion';

describe('css modules inclusion', () => {
it('renders without crashing', () => {
const div = document.createElement('div');
ReactDOM.render(<CssModulesInclusion />, div);
Copy link
Contributor

Choose a reason for hiding this comment

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

How should CSS modules be interpreted by Jest? I would think that http://facebook.github.io/jest/docs/webpack.html#mocking-css-modules is a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added in the recommend identity-obj-proxy method in the jest config. Still need to test properly.

});
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
.cssModulesInclusion {
background: darkblue;
color: lightblue;
}
1 change: 1 addition & 0 deletions packages/react-scripts/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@
"file-loader": "1.1.6",
"fs-extra": "5.0.0",
"html-webpack-plugin": "2.30.1",
"identity-obj-proxy": "3.0.0",
"jest": "22.1.1",
"object-assign": "4.1.1",
"postcss-flexbugs-fixes": "3.2.0",
Expand Down
6 changes: 5 additions & 1 deletion packages/react-scripts/scripts/utils/createJestConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,13 @@ module.exports = (resolve, rootDir, isEjecting) => {
'config/jest/fileTransform.js'
),
},
transformIgnorePatterns: ['[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs)$'],
transformIgnorePatterns: [
'[/\\\\]node_modules[/\\\\].+\\.(js|jsx|mjs)$',
'^.+\\.module\\.css$',
],
moduleNameMapper: {
'^react-native$': 'react-native-web',
'^.+\\.module\\.css$': 'identity-obj-proxy',
},
moduleFileExtensions: [
'web.js',
Expand Down
46 changes: 46 additions & 0 deletions packages/react-scripts/template/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ You can find the most recent version of this guide [here](https://github.com/fac
- [Importing a Component](#importing-a-component)
- [Code Splitting](#code-splitting)
- [Adding a Stylesheet](#adding-a-stylesheet)
- [Adding a CSS Modules stylesheet](#adding-a-css-modules-stylesheet)
- [Post-Processing CSS](#post-processing-css)
- [Adding a CSS Preprocessor (Sass, Less etc.)](#adding-a-css-preprocessor-sass-less-etc)
- [Adding Images, Fonts, and Files](#adding-images-fonts-and-files)
Expand Down Expand Up @@ -513,6 +514,51 @@ In development, expressing dependencies this way allows your styles to be reload

If you are concerned about using Webpack-specific semantics, you can put all your CSS right into `src/index.css`. It would still be imported from `src/index.js`, but you could always remove that import if you later migrate to a different build tool.

## Adding a CSS Modules stylesheet

Copy link
Contributor

Choose a reason for hiding this comment

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

We should comment this section out before landing as it would be too confusing.

This project supports [CSS Modules](https://github.com/css-modules/css-modules) alongside regular stylesheets using the **[name].module.css** file naming convention. CSS Modules allows the scoping of CSS by automatically creating a unique classname of the format **[dir]\_\_[filename]___[classname]**.

An advantage of this is the ability to repeat the same classname within many CSS files without worrying about a clash.

### `Button.module.css`

```css
.button {
padding: 20px;
}
```

### `another-stylesheet.css`

```css
.button {
color: green;
}
```

### `Button.js`

```js
import React, { Component } from 'react';
import './another-stylesheet.css'; // Import regular stylesheet
import styles from './Button.module.css'; // Import css modules stylesheet as styles

class Button extends Component {
render() {
// You can use them as regular CSS styles
return <div className={styles.button} />;
}
}
```
### `exported HTML`
No clashes from other `.button` classnames

```html
<div class="src__Button-module___button"></div>
```

**This is an optional feature.** Regular html stylesheets and js imported stylesheets are fully supported. CSS Modules are only added when explictly named as a css module stylesheet using the extension `.module.css`.

## Post-Processing CSS

This project setup minifies your CSS and adds vendor prefixes to it automatically through [Autoprefixer](https://github.com/postcss/autoprefixer) so you don’t need to worry about it.
Expand Down