Disable PostCSS-custom-properties in development

[alshakero]

Build performance improvement

Yesterday, using Webpack ProfilingPlugin, I did an audit for our build performance to look for low hanging fruits, and did I find one.

It turns out there is a single line in postcss-custom-properties that is more than doubling our yarn start time 🤯 .

Ironically, we never make any use of this line. It detects postcss-custom-properties: ignore next|off CSS comments to disable the plugin for the block that contains this comment. We don't have any of these comments so we're wasting a lot of time for nothing.

I spent some time trying to make the package faster by using some crafted string manipulation logic instead of a regex. But it never got considerably faster. The isBlockIgnored is called a lot, it seems.

After a second thought, it occured to me that CSS custom properties are supported everywhere now except in IE11. IE11 is not supported in our development build anyways, so compiling CSS custom properties down to static values is a total waste of time in development, with the only benefit of shrinking the difference between our prod and development builds. I think slashing the time by half is more than worth this sacrifice.

Testing instructions

The perf difference

1. Run yarn start on master and time it until you see Ready! You can load http://calypso.localhost:3000/ now. Have fun!.
2. Do the same in this branch and the time should be less than half 🎊

On my machine it went down from 2:46 to 1:01.

The CSS outcome

1. Delete your /public folder to make sure no CSS remnants are there.
2. Run yarn start.
3. Play with /new, /read, etc... everything should be styled normally.

[matticbot]

Test live: https://calypso.live/?branch=update/disable-css-custom-props-in-dev

[matticbot]

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Webpack Runtime (~31 bytes added 📈 [gzipped])

name      parsed_size           gzip_size
manifest       -568 B  (-0.3%)      +31 B  (+0.1%)

Webpack runtime for loading modules. It is included in the HTML page as an inline script. Is downloaded and parsed every time the app is loaded.

Sections (~13 bytes removed 📉 [gzipped])

name    parsed_size           gzip_size
reader       -384 B  (-0.1%)      -13 B  (-0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Async-loaded Components (~1714 bytes removed 📉 [gzipped])

name                                parsed_size           gzip_size
async-load-reader-search-stream          -170 B  (-0.2%)     -731 B  (-3.3%)
async-load-reader-following-manage       -170 B  (-0.2%)     -731 B  (-2.5%)
async-load-design-blocks                 -112 B  (-0.0%)      -45 B  (-0.0%)
async-load-reader-site-stream             -50 B  (-0.2%)      -95 B  (-1.3%)
async-load-reader-feed-stream             -50 B  (-0.2%)     -112 B  (-1.6%)

React components that are loaded lazily, when a certain part of UI is displayed for the first time.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

[jsnajdr]

Wow, does grepping all the CSS rules really take that long? I can't believe it 🙂

There are also other postcss.config.js files in the repo that might benefit from a similar patch:

• apps/notifications/postcss.config.js
• packages/calypso-build/postcss.config.js

The config module is probably not available in these, so we need to pass down the "want custom properties" flag in some other way. I remember @ockham did some improvements in these files in the last few months.

[jsnajdr]

I think we can make this condition even more precise. We need postcss-custom-properties only for browsers that don't support CSS variables, and that corresponds 1:1 with the fallback build for IE11, doesn't it?

The evergreen build can omit postcss-custom-properties even in production.

And it can probably be also omitted from the desktop build, after checking that the Electron that we currently use bundles a new enough Chrome. And that's virtually 100% certain, because CSS properties are a rather old feature at this moment.

@sgomes is the expert on our dual builds, might have some other suggestions.

[sgomes]

All of our evergreen browsers definitely support CSS custom properties, so you can certainly make use of that here. You'll need something like what we do in webpack.config.js:

const isCalypsoClient = process.env.BROWSERSLIST_ENV !== 'server';
const isDesktop = calypsoEnv === 'desktop' || calypsoEnv === 'desktop-development';
const defaultBrowserslistEnv = isCalypsoClient && ! isDesktop ? 'evergreen' : 'defaults';
const browserslistEnv = process.env.BROWSERSLIST_ENV || defaultBrowserslistEnv;

if ( browserslistEnv === 'defaults' ) {

That said, I don't like the duplication of logic. What do you thing is the best way of handling things here, @jsnajdr? Should webpack.config.js set the BROWSERSLIST_ENV environment variable to the value of browserslistEnv so that subprocesses like postcss can reuse it?

[jsnajdr]

The webpack config can pass some arbitrary options down to the postcss-loader and in turn to the postcss.config.js file. They call it context.

The code that configures the SassLoader will look like this:

SassConfig.loader( {
  postCssConfig: {
    path: __dirname,
    ctx: {
      transformCssProperties: isFallbackBuild
    },
  },
  ...
} )

Then the postcss.config.js file can use the context: the ctx object is exposed as options:

module.exports = ( { options } ) => {
  const { transformCssProperties } = options;
  // return config object
}

I think this all is a good tool for this PR. The messy logic stays only at one place (the webpack config).

[sgomes]

Yup, that's a lot better than the environment variable! 👍

[jsnajdr]

There is too much unneeded duplication in this code. Can it be like this?

module.exports = () => {
  const plugins = {
    autoprefixer: {},
  };

  if ( wantCustomProps ) {
    plugins[ 'postcss-custom-properties' ] = { ... };
  }

  return { plugins };
}

[alshakero]

Addressed all

[blowery]

Timings i saw for yarn distclean; yarn; time yarn start:

before: yarn start 154.78s user 15.46s system 93% cpu 3:01.47 total

after: yarn start 158.08s user 15.82s system 190% cpu 1:31.14 total

The "total" times here are likely a touch off, as I'm just control-c'ing my way out to get them. But in the right ballpark.

The main webpack build took 75346ms on the branch, 132917ms on the base commit. Very nice!

Looks like this feature came to postcss in v8.0.10. When did we pick that up?

Should also report this back up to the postcss folks.

[alshakero]

There is a regression in IE. CSS vars are still there

https://hash-3b75df7a3b4cb9d293a6c3b934760d4f79fcf14d.calypso.live/calypso/fallback/entry-gutenboarding.cb2a1643ba37d01560e7.min.css

[sgomes]

There is a regression in IE. CSS vars are still there

browserslistEnv will not have 'fallback' but rather 'defaults' for fallback builds, since that is the name of the browserslist configuration. That's likely the issue you're seeing.

The line should be changed to:

transformCssProperties: browserslistEnv === 'defaults',

[sirbrillig]

Anecdotally, this cuts calypso build time for me from 6 minutes to 3. 🎉
(Macbook Pro 2017)

[sgomes]

The new changes look good! 👍

forceFallback=1 shows both CSS custom properties and the precomputed color, as expected, whereas no forceFallback doesn't show precomputed values, only CSS custom properties.

[sgomes]

@nsakaimbo: Could you confirm that the desktop app still works correctly with these changes? We're looking for any errors in styles, most likely the colours being wrong.

[nsakaimbo]

Desktop e2e failures in the build artifact - some sort of runtime error. It's happening consistently, although I don't think it's related to changes in this branch. Still looking...

https://app.circleci.com/pipelines/github/Automattic/wp-desktop/14951/workflows/083203e2-0f53-4f50-87d9-731eff95c322/jobs/63125/artifacts

[nsakaimbo]

Runtime error in the desktop app:

Uncaught (in promise) Error: Loading chunk 193 failed.
(error: http://127.0.0.1:41050/calypso/fallback/193.js)
    at Function.__webpack_require__.e (entry-main.js:2)
    at entry-main.js:2
    at tryCatch (126.js:2)
    at Generator._invoke (126.js:2)
    at Generator.forEach.prototype.(anonymous function) [as next] (http://127.0.0.1:41050/calypso/fallback/126.js:2:626963)
    at asyncGeneratorStep (126.js:2)
    at _next (126.js:2)

Unclear what's happening here, but I do see it in other Calypso branches as well. (It's not happening in wp-desktop develop branch, only in more recent Calypso PRs. So I'm guessing something was introduced since last submodule update that's driving this error.)

[nsakaimbo]

@sgomes @alshakero This change looks great, except I'm unable to verify what it looks like as it seems latest Calypso isn't working in the desktop app (again - see the strange runtime error above). Unfortunately I'm unable to provide an assessment of whether there may be any unintended side effects here.

[jsnajdr]

I did some debugging and I think that the postcss-custom-properties plugin has some great optimization opportunities that could speed up even the production build significantly.

The real problem is not that the isBlockIgnored regexp check would be that inefficient, but rather that the surrounding code is called 700k+ times during the webpack build. The number of calls that are really needed is 3 🙂

We use the importFrom option that loads some common CSS variables from a file in the calypso-color-schemes package. The postcss-custom-properties plugin reads and parses that file every time when creating an instance of the plugin.

And every CSS file needs its own instance of the plugin, because the PostCSS configuration is dynamic: every file can have a different chain of directory-local postcss.config.js files, and the config can be a function that returns a different config for each file.

Calypso webpack build creates 850 instances of the plugin, as that's the number of *.scss files to process. Every instance does its own importFrom processing, i.e., reading the file from disk, parsing it, ignoring the blocks that should be ignored, and extracting the found CSS variables into an object. The getCustomPropertiesFromImports results should be memoized, as they are always the same.

The calypso-color-schemes.css file has three :root selectors, hence the three necessary calls to isBlockIgnored.

Also, for every CSS variable in a block like this:

:root {
  --studio-white: #fff;
  --studio-black: #000;
  /* ... 170 more variables like this ... */
}

the isBlockIgnored function is called on the parent block (i.e., the :root selector), further inflating the number of calls up to hundreds of thousands.

A postcss-custom-properties patch that adds some memoizations and optimizations here and there could have a huge impact.

[alshakero]

Thanks for digging deep! This sounds great. If no one picked this up, I'll work on it on the weekend. I think we might need to create some sort of a benchmark to convince the plugin folks about the issue's significance. And to be able to compare before and after our PR.

[jsnajdr]

I think we might need to create some sort of a benchmark to convince the plugin folks about the issue's significance.

My experience is that performance improvements usually don't need much convincing. Calypso build timings before and after should be enough.

There are two improvements that I expect to be impactful:

1. The more impactful one: in getCustomPropertiesFromImports, memoize the functions that convert a file name (string) to an object with custom properties, so that they actually read and process each file only once.
2. The less impactful one: try to reduce number of calls to isBlockIgnored by removing redundant calls where for each child, the parent node is tested repeatedly in a loop.

[jsnajdr]

@nsakaimbo I can't reproduce the runtime error in the wp-desktop app. Using the latest develop branch of wp-desktop, checking out the update/disable-css-custom-props-in-dev branch in the calypso/ submodule, running make and opening the app in releases/mac. I can login and use the app just fine.

Do I need to go to any specific section to reproduce?

[ockham]

Thanks a ton @alshakero and @jsnajdr for your investigations! ✨ It'd be great if we could fix this upstream so that other A8c projects that use postcss-custom-properties through calypso-build (such as Jetpack) could also profit from the update.

(It would be otherwise conceivable to add an option to calypso-build to disable postcss-custom-properties for them, but that would require some hacks that are against the spirit of calypso-build, which looks for a build tool's own config file rather than adding its own wraparound option -- see e.g.)

[nsakaimbo]

Hi, @jsnajdr - I haven't been able to reproduce this locally, so at the moment I'm trying to "bisect" in CI: link

I'm still testing, but right now the culprit seems to be d46489e.

[nsakaimbo]

Update: been trying to triage this and I think I might have narrowed down the break in the desktop app. Created a new issue with details: #42898

cc: @belcherj @griffbrad

[alshakero]

Thanks @nsakaimbo! Do I understand correctly that wp-desktop fails on Calypso#master too? Can we merge this given it's inconsequential to that issue?

[sgomes]

Thanks @nsakaimbo! Do I understand correctly that wp-desktop fails on Calypso#master too? Can we merge this given it's inconsequential to that issue?

Answering for @nsakaimbo: yes, wp-desktop currently fails on master, which means it shouldn't affect this issue at all. The wp-desktop failure is being tracked separately.

[alshakero]

Alright, so I created an issue to triage the plugin and I'll merge this.