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

Option to turn on deduplication for @import #1233

Closed
jasongrout opened this issue Nov 19, 2020 · 19 comments
Closed

Option to turn on deduplication for @import #1233

jasongrout opened this issue Nov 19, 2020 · 19 comments

Comments

@jasongrout
Copy link

jasongrout commented Nov 19, 2020

  • Operating System: macOS Catalina
  • Node Version: 14
  • NPM Version: 6.14
  • webpack Version: 5.4
  • css-loader Version: 4.3.0

Feature Proposal

Since 3.3.1, CSS @import modules are not deduplicated (removed in 1fb5134, for example). Deduplication was reenabled later, but only for CSS modules (see #1040 (comment) and #1044, which tried to reenable this for @import).

We'd like an option to turn back on deduplication for @import. For example:

module.exports = {
  module: {
    rules: [
      {
        test: /\.css$/i,
        loader: "css-loader",
        options: {
          import: {
            deduplication: true
          },
        },
      },
    ],
  },
};

We recognize in general this affects the order of css on the page, so it probably should default to false. However, providing this option can drastically improve applications that use lots of @import statements to include well-structured css with lots of duplicates.

Feature Use Case

In our case (JupyterLab), we have over 100 packages bundled together with webpack, many of which have css files. Each package uses @import to include the css of its dependencies in its main css file, but no package puts css on the page (i.e., no package imports its css into its javascript files). The top-level application is responsible for putting all the css on the page, and it accomplishes this with a single css file that @imports its direct dependency's css files. In essence, we have a dependency graph of css files which the top-level application is responsible for getting onto the page. For the application as a whole, we have many cases where a particular dependency css file is @imported dozens of times through this dependency graph of css files. This worked fine up through css-loader 3.3.1, since deduplication guaranteed that any given module's css file was only actually included once on the page, in a topographically correct order.

However, css-loader over version 3.3.1 does not do this deduplication, so we end up with thousands of DOM style tags, most of which are duplicates of others (for example, one dependency's CSS is on the page dozens of times). Our application slows to a crawl, and the startup time is at least 10x longer. More specifically, with css-loader 4.3.0, we end up with 23,151 style DOM elements on the page (without deduplication), but when I manually change the css-loader code to reenable deduplication for @import, this shrinks to 175 style DOM elements (and application load times are normal again). As it is, we won't be able to upgrade past css-loader 3.3.1 without either a major restructuring of our css (possibly losing the benefits of our approach) or abandoning or forking css-loader.

Essentially, we would love an option to effect this change:

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, dedup: options.deduplicate });
 
               // eslint-disable-next-line no-continue
               continue;
             }
 
-            options.api.push({ url, media, index });
+            options.api.push({ url, media, index, dedup: options.deduplicate });
           }
         },
       };

If you are okay with merging such an option, I can also work on an implementation, if that helps.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 19, 2020

Sorry no, it is out of spec of official CSS, we want to move CSS pipeline into webpack core, so we will not support not official approaches, also it is not safe (in very many cases)

@alexander-akait
Copy link
Member

This worked fine up through css-loader 3.3.1, since deduplication guaranteed that any given module's css file was only actually included once on the page, in a topographically correct order.

It is not truth 😄

@jasongrout
Copy link
Author

Thanks for the very quick feedback. Do you happen to have any thoughts off the top of your head how to handle a complex dependency graph of css files @importing each other without bogging down with huge numbers of style tags?

@jasongrout
Copy link
Author

This worked fine up through css-loader 3.3.1, since deduplication guaranteed that any given module's css file was only actually included once on the page, in a topographically correct order.

It is not truth 😄

That's good to know too. Since our imports were done in dependency order (i.e., we @import our dependency's css before our own), and the deduplication seemed to be structured such that the first time a module was seen was the time it was put on the page, it would be sorted correctly.

@jasongrout
Copy link
Author

Thanks for the very quick feedback. Do you happen to have any thoughts off the top of your head how to handle a complex dependency graph of css files @importing each other without bogging down with huge numbers of style tags?

To be clear, a typical situation for us is for css files A, B, and C to all import D, and A imports B and C. Without deduplication, D ends up on the page 3 times. Ideally, we'd like D to end up on the page first once, then B and C in either order, then A.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 19, 2020

Thanks for the very quick feedback. Do you happen to have any thoughts off the top of your head how to handle a complex dependency graph of css files @importing each other without bogging down with huge numbers of style tags?

Honestly, it's hard for me to say this without any example, at least minimal

That's good to know too. Since our imports were done in dependency order (i.e., we @import our dependency's css before our own), and the deduplication seemed to be structured such that the first time a module was seen was the time it was put on the page, it would be sorted correctly.

Yep, you can do it in safe way If you put it in the right order, but it is still unsafe, we try to avoid to adding options which can lead to unsafe situations, when something is very specific we recommend to create own loaders/plugins

To be clear, a typical situation for us is for css files A, B, and C to all import D, and A imports B and C. Without deduplication, D ends up on the page 3 times. Ideally, we'd like D to end up on the page first once, then B and C in either order, then A.

This is how CSS works, you can experiment without webpack and you will that it is exactly what happens in the browser

Maybe you need simplify your graph and move some styles to global

@jasongrout
Copy link
Author

jasongrout commented Nov 19, 2020

Yep, you can do it in safe way If you put it in the right order, but it is still unsafe, we try to avoid to adding options which can lead to unsafe situations, when something is very specific we recommend to create own loaders/plugins

I think this is the crux of the issue. CSS in general is kind of a mess with a global namespace, but if you maintain structure things can be safe and efficient. It sounds like we should probably either do the dependency sort ourselves (perhaps using postcss to parse the css files?), or fork css-loader to create our own loader for our more structured situation.

Thanks for the feedback.

@alexander-akait
Copy link
Member

It sounds like we should probably either do the dependency sort ourselves (perhaps using postcss to parse the css files?), or fork css-loader to create our own loader for our more structured situation.

Honestly, you can try postcss-import and implement logic for merging CSS on own side and keep only necessary @imports or even without @import, it sounds not easy and not hard

fork css-loader to create our own loader for our more structured situation

It is also solutions, but in the future everything may change, so maintaining own fork is not the best idea for long term

@jasongrout
Copy link
Author

Honestly, you can try postcss-import and implement logic for merging CSS on own side and keep only necessary @imports or even without @import, it sounds not easy and not hard

Thanks for the tip on using postcss-import

It is also solutions, but in the future everything may change, so maintaining own fork is not the best idea for long term

In general, forking is a last resort, hence this issue asking about reenabling it in upstream first. As it is, we haven't been able to upgrade for a while past when the deduplication was removed, so that's also been a maintenance burden.

@jasongrout
Copy link
Author

Thanks for the tip on using postcss-import

From the docs, it looks like postcss-import actually skips duplicate css files by default, which looks like it may be exactly the default behavior we want: https://github.com/postcss/postcss-import#skipduplicates.

@alexander-akait
Copy link
Member

alexander-akait commented Nov 19, 2020

From the docs, it looks like postcss-import actually skips duplicate css files by default, which looks like it may be exactly the default behavior we want: https://github.com/postcss/postcss-import#skipduplicates.

https://github.com/postcss/postcss-import/blob/master/index.js#L227

not sure, you need to dedupe it between modules

@jasongrout
Copy link
Author

not sure, you need to dedupe it between modules

At first glance, it looks to me like the scope there is referring to the media scope (e.g., @import file is root scope, but @import file print; is the different 'print' scope). So from that code (and the code below dealing with hashes), I would expect two modules containing @import url("~module/index.css") to have those imports deduplicated, and furthermore two modules importing completely separate filenames, but with the same content (e.g., a common normalization css file) to also be deduplicated unless that file contains imports itself.

In light of this default deduplicating behavior from postcss-import, showing that it is indeed a common thing, it would be great if you would reconsider. But I also understand if you don't want to support import deduplication, and we can work on our own preprocessor or loader for our situation.

@alexander-akait
Copy link
Member

In light of this default deduplicating behavior from postcss-import, showing that it is indeed a common thing

Sometimes developers do strange things and then looking for solutions to deal with these strange things 😄

Honestly, I'd take a look at some kind of minimal working example, maybe this will allow you to see how it better to solve and improve docs about it, but we have one bug with deduplication in style-loader (very very very edge case), and to make sure it's not it

@jasongrout
Copy link
Author

I posted a minimal toy working example at https://github.com/jasongrout/cssimports

@jasongrout
Copy link
Author

jasongrout commented Nov 20, 2020

we have one bug with deduplication in style-loader (very very very edge case), and to make sure it's not it

Is there an open issue I can look at?

Edit: is it this issue? webpack-contrib/style-loader#450 (comment)

@alexander-akait
Copy link
Member

Thanks, I will look at this in near future (focus on webpack-dev-server and webpack-cli in this week)

@jasongrout
Copy link
Author

Thanks, and thanks for all of your work in the webpack ecosystem!

@alexander-akait
Copy link
Member

@jasongrout And yes, we have 2 edge cases in link what you provide

@alexander-akait
Copy link
Member

Closing due to inactivity. Thank you and feel free to feedback

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants