Build CSS assets with Webpack #26820

jsnajdr · 2018-08-22T08:23:20Z

Latest status:
This PR evolved from something that was a proof of concept into a finished production-ready PR. See #26820 (comment) for more info about the final state of things.

Original description from the proof-of-concept era:
This is a proof of concept of migrating the CSS build pipeline to Webpack. Moves a few components' styles to import './style.scss' in the JS module, and makes sure that the CSS is built with Webpack and the stylesheets are included in the server-generated index.html on initial load.

Code splitting also seems to work very well: Webpack automatically creates CSS chunks for async-loaded sections and loads the stylesheets together with scripts when loading the section. Our custom code for "sections CSS" can be eventually removed.

See individual commit messages for details.

Largest missing piece is support for RTL. Completely ignored at the moment. I'm afraid we'll need to write a custom Webpack plugin, a nontrivial one, to make it work as we need it:

generate both LTR and RTL versions of each CSS asset
choose the right stylesheet on initial load
load the right stylesheet when async-loading a section

If we wanted to do a gradual migration and keep both CSS pipelines in place for some time, there is one gotcha I discovered that makes this very fragile: we have some CSS rules where specificity is determined by rule order and nothing else. Example:

.button {
  font-weight: bold;
}
.header-button {
  font-weight: normal;
}

<button class="button header-button">

What is the resulting font-weight of the button? It depends on the order of the rules. The last one wins. It's very easy to break this when moving stylesheets around, as the order can change.

Another todo: patch the other index files in client/document (desktop, browsehappy, support-user) to also include links to the right stylesheets.

How to test:
Run Calypso both in debug (npm run start) and production (CALYPSO_ENV=production npm run build) modes and verify that all stylesheets are loaded as expected.

matticbot · 2018-08-22T08:23:22Z

Test live: https://calypso.live/?branch=try/webpackify-css

simison · 2018-08-22T08:27:02Z

webpack.config.js

@@ -191,6 +194,10 @@ function getWebpackConfig( { externalizeWordPressPackages = false } = {}, argv )
 							loader: 'sass-loader',
 							options: {
 								includePaths: [ path.join( __dirname, 'client' ) ],
+								data: `
+									@import '${ path.join( __dirname, 'assets/stylesheets/shared/_colors.scss' ) }';
+									@import '${ path.join( __dirname, 'assets/stylesheets/shared/_utils.scss' ) }';


It's enough to import just _utils.scss here because it in turn imports colors:

wp-calypso/assets/stylesheets/shared/_utils.scss

Line 3 in a20f92f

@import '../shared/colors'; // import all of our wpcom colors

Thanks for pointing this out. Addressed in 3bbffee

simison · 2018-08-22T08:43:45Z

webpack.config.js

@@ -113,6 +113,9 @@ const wordpressExternals = ( context, request, callback ) =>
 */
 // eslint-disable-next-line no-unused-vars
 function getWebpackConfig( { externalizeWordPressPackages = false } = {}, argv ) {
+	const cssFilename =
+		isDevelopment || calypsoEnv === 'desktop' ? 'style-[name].css' : 'style-[name].[chunkhash].css';


Do we necessarily need style- prefix? The .css extension already tells us it's a stylesheet.

We don't really need the prefix, it's just that our .gitignore file doesn't ignore all /public/*.css files, but only ones that match certain patterns: /public/style*.css. Easy to change, of course, but I choose the path of least resistance :)

simison · 2018-08-22T08:51:29Z

webpack.config.js

@@ -113,6 +113,9 @@ const wordpressExternals = ( context, request, callback ) =>
 */
 // eslint-disable-next-line no-unused-vars
 function getWebpackConfig( { externalizeWordPressPackages = false } = {}, argv ) {
+	const cssFilename =


Can we make this configurable? That would help SDK because if I now run:

npm run sdk:gutenberg -- --editor-script=client/gutenberg/extensions/todo/block.js

That would compile a file client/gutenberg/extensions/todo/build/style-todo-editor.css (with style-) instead of client/gutenberg/extensions/todo/build/todo-editor.css

Perhaps something like this:

function getWebpackConfig( { cssFilename, externalizeWordPressPackages = false } = {}, argv ) { if ( ! cssFilename ) { cssFilename = isDevelopment || calypsoEnv === 'desktop' ? 'style-[name].css' : 'style-[name].[chunkhash].css'; }

And in SDK:

const baseConfig = getBaseConfig( { cssFilename: '[name].css', externalizeWordPressPackages: true } );

simison · 2018-08-22T08:58:12Z

Jest is failing for this because it doesn't know what to do with scss imports.

I solved this in another PR (#26683) by mocking style imports, so you might want to look into that solution: https://github.com/Automattic/wp-calypso/blob/c72ff9c44422589813bdb43f270e619827419aef/test/test/helpers/styles/README.md

https://jestjs.io/docs/en/webpack#handling-static-assets

ockham · 2018-08-22T09:55:26Z

If we wanted to do a gradual migration and keep both CSS pipelines in place for some time, there is one gotcha I discovered that makes this very fragile: we have some CSS rules where specificity is determined by rule order and nothing else. Example:
.button {
  font-weight: bold;
}
.header-button {
  font-weight: normal;
}
<button class="button header-button">
What is the resulting font-weight of the button? It depends on the order of the rules. The last one wins. It's very easy to break this when moving stylesheets around, as the order can change.

Do you have any grasp how many of those we have? Is this something we can fix and discourage Calypso-wide?

simison · 2018-08-22T10:06:25Z

Is this something we can fix and discourage Calypso-wide?

If this becomes a blocker, can we scope this PR down to not contain any style imports in some of the JS files for now? I.e. just keep the build and global _utils.scss.

jsnajdr · 2018-08-22T11:10:23Z

Do you have any grasp how many of those we have? Is this something we can fix and discourage Calypso-wide?

I have no idea. I discovered the button header-button issue by looking at the /plugins/manage page and seeing that a button styling was broken. There will surely be many other styles like this, but I don't know how to find them reliably.

It can and should be fixed by making the selectors more specific: instead of declaring .header-button styles, change them to .button.header-button.

If this becomes a blocker, can we scope this PR down to not contain any style imports in some of the JS files for now?

Like committing the Webpack CSS build pipeline, but not actually using it for anything? Well that's possible, but suboptimal :)

jsnajdr · 2018-08-23T08:46:35Z

I pushed an update to this PR that adds RTL support. There are three steps:

Use webpack-rtl-plugin to generate RTL and LTR versions of each CSS asset. cssnano minification has been removed from the postcss step, as it needs to happen at the very end, on both "forked" versions of the CSS separately. The webpack-rtl-plugin takes care of that internally, configured with the minify option.
Use mini-css-extract-plugin-with-rtl, a fork of the official version (thanks @ockham for finding it!). It has a patched runtime dynamic loader that looks at the document.dir global variable every time an async CSS chunk is being loaded and requests the appropriate version of the asset.
Patch the code that generates the index.html on server and ships <link rel="stylesheet"/> tags in the initial document. Make it look at the user's isRTL flag and send the right version of the stylesheet.

Step 3 works only when user bootstrapping is in action. If the server is not capable to retrieve the authenticated user's locale info, it will always send the LTR version. There is code in the client (search for setLocaleInDOM) that switches the stylesheets at runtime at the time when the locale becomes known. That runtime switch doesn't work yet in this PR.

ockham · 2018-08-23T18:01:05Z

webpack.config.js

@@ -12,7 +12,8 @@ const fs = require( 'fs' );
 const path = require( 'path' );
 const webpack = require( 'webpack' );
 const AssetsWriter = require( './server/bundler/assets-writer' );
-const MiniCssExtractPlugin = require( 'mini-css-extract-plugin' );
+const MiniCssExtractPlugin = require( 'mini-css-extract-plugin-with-rtl' );


Should we rename the import to reflect the WithRtl part?

Hmm, I'd rather keep the name without a suffix. It's a forked plugin that will likely be merged into upstream sooner or later.

We might need to actively lobby the upstream project for this to happen tho...

We might need to actively lobby the upstream project for this to happen tho...

I just got involved by commenting on an issue and by filing another, somewhat related one.

ockham · 2018-08-23T18:04:11Z

This seems to be working pretty well!

In order to divide & conquer, would it make sense to move the webpack config changes (that essentially enable RTL) to a separate PR (which will only affect bundles built by the SDK for now), and to sort out the remaining questions for Calypso as a whole separately?

tyxla · 2018-08-24T07:54:10Z

webpack.config.js

@@ -113,6 +114,9 @@ const wordpressExternals = ( context, request, callback ) =>
 */
 // eslint-disable-next-line no-unused-vars
 function getWebpackConfig( { externalizeWordPressPackages = false } = {}, argv ) {
+	const cssFilename =
+		isDevelopment || calypsoEnv === 'desktop' ? 'style-[name].css' : 'style-[name].[chunkhash].css';


With this when we build a Gutenberg extension from the SDK for production, the output CSS files will contain the chunkhash in the name:

NODE_ENV=production npm run sdk:gutenberg -- --editor-script=client/gutenberg/extensions/editor-notes/index.js

Perhaps it's a good idea to keep it only style-[name].css for the SDK, regardless of whether we're building for development or production.

Changes the ignore rules to ignore all CSS and CSS sourcemap files in `/public`. There are no version-controlled CSS files in that directory and the current rules are overly specific. They were created a long time ago in pre-OSS era. Removes unnecessary constraints on naming of CSS chunks in #26820.

jsnajdr · 2018-09-19T13:44:19Z

Hear hear, this PR is ready for review and introduces the first bit of production CSS built with Webpack into Calypso.

What are the changes in this PR?

Webpack "assets" for the app entrypoint or a chunk used to be just JS files, but now there are three types of them: JS, CSS.LTR and CSS.RTL. When creating the index.html document on server, there is a new code that includes links to these assets correctly: JS as <script> tags, CSS of the appropriate flavor as <link rel=stylesheet> tags.

I created a fork of the mini-css-extract-plugin-with-rtl that includes a data-webpack=true attribute on each <link rel=stylesheet> tag. We'll use that attribute to find <link> tags where we want to switch stylesheets from LTR to RTL. It's unfortunate that we now have our own fork of something that's already a fork. Let's work on integrating all the changes back into upstream, but I don't expect quick results here.

When switching locale at runtime, there's new code (switchWebpackCSS) to switch CSS assets coming from Webpack, too. Try the testing instructions from #27236 on the login page to see how it works.

Finally I converted the login section to Webpack CSS as our first Webpack-generated production CSS.

blowery · 2018-09-19T19:02:55Z

client/lib/i18n-utils/switch-locale.js

+function switchWebpackCSS( isRTL ) {
+	const currentLinks = document.querySelectorAll( 'link[rel="stylesheet"][data-webpack]' );
+
+	return map( currentLinks, async currentLink => {


Do we need to return an array of promises? Doesn't appear it's used...

Hmm, that's right, nobody waits for the result. That's true for the whole switchLocale function that does a lot of async updates -- classic CSS, webpack CSS, loading the translation file... We just call switchLocale and never look back.

I think it's still "good manners" to return such a promise, so I'm reluctant to remove the code. 🎵 We shall surely use it one day 🎵

blowery · 2018-09-19T19:20:11Z

package.json

@@ -132,7 +132,7 @@
    "lru": "3.1.0",
    "lunr": "2.3.3",
    "marked": "0.5.0",
-    "mini-css-extract-plugin-with-rtl": "0.1.3",
+    "mini-css-extract-plugin-with-rtl": "github:jsnajdr/mini-css-extract-plugin-with-rtl",


can we either publish this fork to npm under a new name or at least move the github repo under Automattic? :)

Moved to the A8c org in e16fb8e

mini-css-extract-plugin-with-rtl-and-webpack-attr is too long a name :)

Server build doesn't need to process any CSS imports -- that's only for Calypso client build. Fixed build errors in Automattic/wp-calypso#26820.

jsnajdr · 2018-09-20T07:47:14Z

The desktop build should become green again after Automattic/wp-desktop#503 is merged.

Server build doesn't need to process any CSS imports -- that's only for Calypso client build. Fixed build errors in Automattic/wp-calypso#26820.

ockham · 2018-09-20T21:58:00Z

client/lib/i18n-utils/switch-locale.js

+			currentLink.parentElement.removeChild( currentLink );
+		}
+	} );
+}


There is some overlap in terms of functionality of this function with what we're doing in DocumentHead to change the document title on the client side when switching between different sections.

I wonder if we could extend DocumentHead to also cover the CSS link case, and then dispatch setDocumentHeadLink when flipping betwwen LTR and RTL. (A word of caution, DocumentHead, and when to change or reset what, has been a source of subtle bugs, due to the many requirements there are for it -- amoung them SSR). However, it might be worth centralizing this kind of logic. (I don't want to block this PR on it though, can't wait to see this one at work 🙂 )

I think I'll pass on this idea 🙂

DocumentHead owns the document title and some of the meta and link elements (but not all). The source of truth is Redux state, both in server and client rendering, and DocumentHead is a client-side tool that makes sure that the DOM is updated.

The script and stylesheet links, on the other hand, are not owned by Redux or any app state. They are owned by webpack. setLocaleInDOM updates these webpack-owned links in a careful cooperation with the webpack runtime.

I think these two very distinct concepts should be mixed together. The only thing they have in common is that the stylesheets live in the head element, that's all.

Okay, fair enough.

ockham · 2018-09-20T22:19:18Z

client/lib/i18n-utils/switch-locale.js

+	return map( currentLinks, async currentLink => {
+		const currentHref = currentLink.getAttribute( 'href' );
+		const newHref = flipRTL( currentHref, isRTL );
+		if ( currentHref === newHref ) {


Can this ever happen, given what flipRTL does?

The logic is "put the URL into the desired state (LTR or RTL)" rather than "toggle the state from the current state to the other one". Maybe flipRTL has a misleading name?

Yeah, might be worth renaming 🤔

Renamed to setRTLFlagOnCSSLink and added doc comment to the function.

ockham · 2018-09-20T22:32:17Z

I tested successfully:

Logged-in mode
Switching to Arabic as UI language in logged in mode
Logged-out mode, landing at /log-in/de
Logged-out mode, landing at /log-in/he, and switching to German

ockham

This tests well and looks good 👍 (Good job on getting blockers out of the way in separate PRs!)

I have one question, I noticed that while our CLI produced stylesheets are loaded with a hash query arg (e.g. /calypso/style.css?v=d247105eb8), that's not the case for the webpack-produced ones in this PR. Shouldn't they also get one?

jsnajdr · 2018-09-21T08:25:06Z

I have one question, I noticed that while our CLI produced stylesheets are loaded with a hash query arg (e.g. /calypso/style.css?v=d247105eb8), that's not the case for the webpack-produced ones in this PR. Shouldn't they also get one?

The stylesheets get a hash in production mode, .e.g.,

https://wordpress.com/calypso/login.360b2c2bc4a2f42c5ded.css

Just like the JS chunks.

In development mode, there is no hash. Neither for JS nor CSS.

ockham

LGTM 👍

…m into index.html Until now, Webpack generated only JS assets, but now there are also CSS. When generating index.html on the server, each type needs a different tag: `<link rel="stylesheet">` vs `<script>`. Divide the assets into groups and insert appropriate tags.

…inks

jsnajdr added [Status] In Progress Build [Project] SDK labels Aug 22, 2018

jsnajdr self-assigned this Aug 22, 2018

jsnajdr requested review from simison, ockham and ehg August 22, 2018 08:23

jsnajdr requested review from dmsnell and a team August 22, 2018 08:23

simison reviewed Aug 22, 2018

View reviewed changes

simison requested review from simison, tyxla and coderkevin August 22, 2018 08:28

simison reviewed Aug 22, 2018

View reviewed changes

jsnajdr force-pushed the try/webpackify-css branch from 4b9abfd to 4cf0cce Compare August 23, 2018 08:34

ockham reviewed Aug 23, 2018

View reviewed changes

simison mentioned this pull request Aug 24, 2018

SDK: import Calypso UI into a Gutenberg extension and scope styles #26683

Closed

tyxla reviewed Aug 24, 2018

View reviewed changes

This was referenced Aug 24, 2018

Git-ignore all CSS files in /public #26862

Merged

Header Button: update CSS selectors to be more specific #26863

Closed

blowery reviewed Sep 19, 2018

View reviewed changes

jsnajdr added a commit to Automattic/wp-desktop that referenced this pull request Sep 20, 2018

Ignore SASS/CSS imports when building desktop

3dca8d6

Server build doesn't need to process any CSS imports -- that's only for Calypso client build. Fixed build errors in Automattic/wp-calypso#26820.

jsnajdr mentioned this pull request Sep 20, 2018

Ignore SASS/CSS imports when building desktop Automattic/wp-desktop#503

Merged

jsnajdr force-pushed the try/webpackify-css branch from e16fb8e to 46e8fc4 Compare September 20, 2018 10:26

ockham reviewed Sep 20, 2018

View reviewed changes

ockham approved these changes Sep 21, 2018

View reviewed changes

jsnajdr force-pushed the try/webpackify-css branch from 46e8fc4 to f382dfa Compare September 21, 2018 11:08

jsnajdr added 7 commits September 21, 2018 13:16

Remove unneeded eslint comment

80a8278

Use a forked css-extract-plugin that adds data-webpack attribute to l…

2b820d5

…inks

Switch stylesheets from Webpack chunks

dcdb2ea

Include link to Webpack main stylesheet in desktop

2154002

Webpackify the login section CSS

b8405ac

Rename and document the flipRTL function

4ea0b45

jsnajdr force-pushed the try/webpackify-css branch from 6a134c5 to 4ea0b45 Compare September 21, 2018 11:20

jsnajdr merged commit 40d13b3 into master Sep 21, 2018

jsnajdr deleted the try/webpackify-css branch September 21, 2018 13:30

matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Sep 21, 2018

jsnajdr mentioned this pull request Sep 24, 2018

Framework: CSS Splitting #19847

Closed

bluefuton mentioned this pull request Sep 24, 2018

Reader full post: remove from section CSS and import style.scss in component instead #27408

Merged

bluefuton mentioned this pull request Nov 27, 2018

Move reader-main to reader/components #28272

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Build CSS assets with Webpack #26820

Build CSS assets with Webpack #26820

jsnajdr commented Aug 22, 2018 •

edited

Loading

matticbot commented Aug 22, 2018

simison Aug 22, 2018

jsnajdr Aug 23, 2018 •

edited

Loading

simison Aug 22, 2018

jsnajdr Aug 22, 2018

simison Aug 22, 2018 •

edited

Loading

simison commented Aug 22, 2018 •

edited

Loading

ockham commented Aug 22, 2018

simison commented Aug 22, 2018

jsnajdr commented Aug 22, 2018

jsnajdr commented Aug 23, 2018

ockham Aug 23, 2018

jsnajdr Aug 24, 2018

ockham Aug 24, 2018

jsnajdr Aug 27, 2018

ockham commented Aug 23, 2018

tyxla Aug 24, 2018

jsnajdr commented Sep 19, 2018

blowery Sep 19, 2018 •

edited

Loading

jsnajdr Sep 19, 2018

blowery Sep 19, 2018

jsnajdr Sep 19, 2018 •

edited

Loading

jsnajdr commented Sep 20, 2018

ockham Sep 20, 2018 •

edited

Loading

jsnajdr Sep 21, 2018

ockham Sep 21, 2018

ockham Sep 20, 2018

jsnajdr Sep 21, 2018

ockham Sep 21, 2018

jsnajdr Sep 21, 2018

ockham commented Sep 20, 2018

ockham left a comment

jsnajdr commented Sep 21, 2018

ockham left a comment

Build CSS assets with Webpack #26820

Build CSS assets with Webpack #26820

Conversation

jsnajdr commented Aug 22, 2018 • edited Loading

matticbot commented Aug 22, 2018

Choose a reason for hiding this comment

jsnajdr Aug 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simison Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

simison commented Aug 22, 2018 • edited Loading

ockham commented Aug 22, 2018

simison commented Aug 22, 2018

jsnajdr commented Aug 22, 2018

jsnajdr commented Aug 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

ockham commented Aug 23, 2018

Choose a reason for hiding this comment

jsnajdr commented Sep 19, 2018

blowery Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsnajdr Sep 19, 2018 • edited Loading

Choose a reason for hiding this comment

jsnajdr commented Sep 20, 2018

ockham Sep 20, 2018 • edited Loading

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

ockham commented Sep 20, 2018

ockham left a comment

Choose a reason for hiding this comment

jsnajdr commented Sep 21, 2018

ockham left a comment

Choose a reason for hiding this comment

jsnajdr commented Aug 22, 2018 •

edited

Loading

jsnajdr Aug 23, 2018 •

edited

Loading

simison Aug 22, 2018 •

edited

Loading

simison commented Aug 22, 2018 •

edited

Loading

blowery Sep 19, 2018 •

edited

Loading

jsnajdr Sep 19, 2018 •

edited

Loading

ockham Sep 20, 2018 •

edited

Loading