Update css-loader, style-loader and mini-css-extract-plugin

[jtpio]

References

Address part of #9297

Code changes

This is an attempt at updating css-loader from 3.x to 5.x, since it started to give peer dependency warnings with webpack 5.

Also update style-loader and mini-css-extract-plugin.

However it seems to be a bit tricky to get it to work locally. Even though it seems to be compiling fine, the browser tab goes into high memory usage pretty quickly.

Looking at previous PRs that updated dependencies, these two look relevant:

• Update dependencies for beta #8921 (comment)
• Update dependencies #7590 (comment)

And arrive to the same conclusion that new versions seem to be causing issues.

User-facing changes

None

Backwards-incompatible changes

Linking to the changelog for css-loader:

• Breaking changes in v4: https://github.com/webpack-contrib/css-loader/blob/8b865fe4a4d708f7269e9f0a238ac8d9fdf85576/CHANGELOG.md#-breaking-changes
• Breaking changes in v5: https://github.com/webpack-contrib/css-loader/blob/db3911be1a797abeb3afd5499684b9e610faacaa/CHANGELOG.md#500-2020-10-13

[jupyterlab-dev-mode]

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link:

[jasongrout]

FYI, I can get profiling in Chrome by opening the browser tools, selecting the performance tab, then hitting the refresh button there which loads your site while profiling. Indeed, it looks like css loader is responsible for the vast majority of the 20 seconds it takes to load with this pr:

[jasongrout]

Confirming #7590 (comment), if I install jlab 3.0.0rc10 in a clean environment, and modify the site-packages staging/package.json to have a resolution of "css-loader": "3.3.1" and a dev dependency of "css-loader": "3.3.1", then do jupyter lab build, then 3.3.1 is installed and everything seems to work great. If I change the dependency to 3.3.2 and rebuild, loading jupyterlab stalls.

[jasongrout]

There is only one commit between v3.3.1 and v3.3.2: webpack-contrib/css-loader@1fb5134, which only modifies one non-test file: webpack-contrib/css-loader@1fb5134?branch=1fb51340a7719b6f5b517cb71ea85ec5d45c1199&diff=split#diff-93e7da4f58ea51ecd7f2f2bc482ea52d49b70591ce175528a68148980dcc1bf9

It looks like one of the things that commit does is eliminate a cache of already-loaded modules, which looks a bit suspicious.

[jasongrout]

Digging into this more, you'll see that the deduplication is back in the latest css-loader codebase, but this time behind a function option (the last dedupe option), which defaults to undefined (i.e., false):

https://github.com/webpack-contrib/css-loader/blob/master/src/runtime/api.js#L25-L61

Indeed looking at our codebase, each style file imports its dependency's style files, so we end up with lots of imports of lumino widgets css, for example. Here is an excerpt from our compiled jlab env/share/jupyter/lab/static/vendors-node_modules_css-loader_dist_cjs_js_node_modules_jupyterlab_application-extension_sty-b276ca.cabba44652bf064016fe.js:

___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_translation_style_index_css__WEBPACK_IMPORTED_MODULE_1__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_lumino_widgets_style_index_css__WEBPACK_IMPORTED_MODULE_2__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_ui_components_style_index_css__WEBPACK_IMPORTED_MODULE_3__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_application_style_index_css__WEBPACK_IMPORTED_MODULE_4__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_property_inspector_style_index_css__WEBPACK_IMPORTED_MODULE_5__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_base_css__WEBPACK_IMPORTED_MODULE_6__.default);


[ a little later in the file, in a different module bundled in the same file]

___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_fortawesome_fontawesome_free_css_all_min_css__WEBPACK_IMPORTED_MODULE_1__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_fortawesome_fontawesome_free_css_v4_shims_min_css__WEBPACK_IMPORTED_MODULE_2__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_translation_style_index_css__WEBPACK_IMPORTED_MODULE_3__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_lumino_widgets_style_index_css__WEBPACK_IMPORTED_MODULE_4__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_ui_components_style_index_css__WEBPACK_IMPORTED_MODULE_5__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_apputils_style_index_css__WEBPACK_IMPORTED_MODULE_6__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_docregistry_style_index_css__WEBPACK_IMPORTED_MODULE_7__.default);
___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_base_css__WEBPACK_IMPORTED_MODULE_8__.default);

and so on. By my search, there are 36 loadings of the lumino widgets css (searching for ___CSS_LOADER_EXPORT___.i(_css_loader_dist_cjs_js_lumino_widgets_style_index_css). Notice that none of these loadings are done with that third parameter, dedupe set to true.

My hypothesis is that we're getting tons of css duplicated on the page, and the browser is having a hard time keeping up with all of the relayouts that is causing.

[jasongrout]

Someone asked if that dedupe parameter could default to true, but never followed up with a reason for it: webpack-contrib/css-loader#1040 (comment)

[jasongrout]

I think this PR would have set imports to be deduplicated by default, but was closed with the comment that @import should not be deduplicated: webpack-contrib/css-loader#1044

Technically I suppose that is correct - order matters in css, so deduplicating messes up that order. However, we have a system that relies on deduplication to function efficiently (i.e., we haven't cared too much about duplication, because things have just happened to work out pretty well). I don't know if there is an easy way to globally set that deduplication for imports to get our previous behavior.

I suppose what we really ought to do is a topological sort of the style file dependency tree, then include the single list of style files. This requires us to either walk the dependency tree (looking for package.json style keys), or to parse the style files for @import statements. Too bad the css importer doesn't do this for us!

[jasongrout]

Too bad the css importer doesn't do this for us!

Well, on the other hand, I suppose it probably would if that dedupe were true - since files are loaded in dependency order, we always know we'll get the styles of our dependencies on the page before our styles, so as long as the modules are deduplicated, the style files would be deduplicated automatically.

[jasongrout]

It looks like css-loader does understand the css-modules composes syntax: https://github.com/css-modules/css-modules#composition, but that would be quite a significant change for us at this point.

Edit: hmm, looking at the documentation for css-modules, it looks like an idea from about 5 years ago that doesn't seem to have a lot of traction after a few years, based on updates to the main examples and implementations in various loaders. I don't think it's a way we want to go.

[jasongrout]

Another thought: the way we used to do things, with each package importing its own css into its javascript, webpack would do the deduplication for us when it deduplicated the js modules, so we wouldn't have faced this issue, I think. However, since we moved to having a parallel dependency graph among the css style files (independent of the dependencies among the js files), with only the very top layer actually putting these css files on the page, we don't get the automatic deduplication for free from webpack module deduplication.

[jasongrout]

I think perhaps a way forward with the css-loader is to contribute a new option for the css loader that would tell it to deduplicate @import statements (i.e., basically do what it used to do). In some respects, it is "incorrect" as far as CSS goes, but in other respects, since it obeys the dependencies among css files, it is "correct" and it would probably be way more efficient as well.

Perhaps something like this (still some more work needed, in case the filter function is given in the import object options:

diff --git a/src/index.js b/src/index.js
index 8cd1f7b..c83b626 100644
--- a/src/index.js
+++ b/src/index.js
@@ -67,7 +67,7 @@ export default async function loader(content, map, meta) {
       mainFiles: ["index", "..."],
       restrictions: [/\.css$/i],
     });
-
+    const deduplicate = typeof options.import === "object" ? options.import.deduplicate : false;
     plugins.push(
       importParser({
         imports: importPluginImports,
@@ -81,6 +81,7 @@ export default async function loader(content, map, meta) {
             this,
             getPreRequester(this)(options.importLoaders) + url
           ),
+        deduplicate
       })
     );
   }
diff --git a/src/plugins/postcss-import-parser.js b/src/plugins/postcss-import-parser.js
index b0698a6..8e73944 100644
--- a/src/plugins/postcss-import-parser.js
+++ b/src/plugins/postcss-import-parser.js
@@ -178,13 +178,13 @@ const plugin = (options = {}) => {
                 });
               }
 
-              options.api.push({ importName, media, index });
+              options.api.push({ importName, media, index, dedupe: options.deduplicate });
 
               // eslint-disable-next-line no-continue
               continue;
             }
 
-            options.api.push({ url, media, index });
+            options.api.push({ url, media, index, dedupe: options.deduplicate });
           }
         },
       };

[jasongrout]

Without that dedupe switch in css-loader, we have 23151 style DOM elements on the page (and a 20-30 second load time in Chrome, I think longer in Firefox). With that deduping in css-loader, we have 175 style DOM elements and a normal load time.

[jasongrout]

I filed webpack-contrib/css-loader#1233 about the css-loader deduplication issue. Let's hope we hear back from them soon.

[jasongrout]

Good news is that the css-loader maintainer was amazingly responsive. Not-so-good news is that it looks like we might have to drop css-loader (or fork it), as reenabling css deduplication seems to be off the table. On the other hand, the css-loader maintainer did give hints that using postcss-import to preprocess our files into a single css file might not be so bad. However, it might cause issues with module sharing if we start combining css ourselves in a preprocessing step.

[jasongrout]

As it stands now, I think we ought to table this for now and take this up after 3.0, since things seem to work right now except for the warning.

After 3.0, given that the css-loader author seems not willing to consider adding a css import deduplication option (because it may introduce problems for general css usage), I think the best way forward in the short term would be to fork css-loader and add the small patch on top for deduplicating imports. We could publish it as @jupyterlab/css-loader, for example. It will be a small maintenance burden on us, but a minimal patch would change two lines, so it probably wouldn't be too hard to keep up to date.

[jasongrout]

More relevant discussion at webpack-contrib/style-loader#450 - with some examples showing that in general you do not want to deduplicate.

[jtpio]

As it stands now, I think we ought to table this for now and take this up after 3.0, since things seem to work right now except for the warning.

That sounds like the most reasonable thing to do for now.

Thanks a lot Jason for the in-depth investigation 👍

[jasongrout]

Note also that upgrading css-loader would solve #6542

[jasongrout]

css-loader is now updated (in #9407). Can we rebase (or close and open a new pr) to update style-loader and mini-css-extract-plugin?

[jtpio]

OK, let's close it then.

Updated this comment to add it to the list: #9297 (comment)

Thanks again for pushing on this!