feat: onEmit configuration option

[jamietre]

This PR contains a:

• bugfix
• new feature
• code refactor
• test update
• typo fix
• metadata update

Motivation / Use-Case

We need to lock the order that CSS appears in assets produced by mini-css-extract-plugin over time. We've implemented this my maintaining a lock file that persists the order of CSS modules when they are first seen, but this requires post-processing of the final list produced by the plugin to enforce the order when the bundles are built.

This pull request adds a hook so consumers can optionally post-process the final set of "used modules" before they are returned to webpack.

Breaking Changes

None

Additional Info

None

[jsf-clabot]

All committers have signed the CLA.

[alexander-akait]

Should be done on webpack level, not on plugin

[alexander-akait]

Please provide repo with use case and describe in README what you have and what you expected

[jamietre]

Please provide repo with use case and describe in README what you have and what you expected

I can extract all the code we're using with this into a standalone repo, but it would be a lot of work; is the description of our use case not clear? I'm not sure how I could do this at the webpack level without just forking the plugin, which I'm trying to avoid.

The order of CSS assets produced by mini-css-extract-plugin is not deterministic over time, as things are added and removed, which can create instability because of CSS conflicts. So we capture the order of assets and add them to a lock file when they're first seen. On future builds, new assets are inserted in to the lock file using an algorithm that determines the best place without altering the order of any existing assets.

So on the first run, the lock file will match the order produced by mini-css-extract-plugin. On future runs, when a new asset is seen, we insert it into the lock file, but never change the order of existing assets in the lock file. (This is where the order will diverge from mini-css-extract-plugin, since nothing prevents future runs from producing CSS bundles in a different order with the current logic in the plugin). We then need to re-order the set to match the order of our lockfile.

[alexander-akait]

@jamietre I can't understand you. Why you need this, need repo to demonstrate problem. Some features out of scope this plugin, maybe better create own plugin for this rare use case

[jamietre]

@jamietre I can't understand you. Why you need this, need repo to demonstrate problem. Some features out of scope this plugin, maybe better create own plugin for this rare use case

I agree the specific problem/our solution is out of scope; that's why I only want to add a generic hook to empower users to add custom logic to the final order of assets produced by mini-css-extract-plugin.

There are many issues related to this problem.

#202
#124
#133

Maybe this should just be another plugin in the pipeline after mini-css-extract-plugin? This seemed like a straightforward solution but perhaps there is a better ways to re-order the assets after they are output by mini-css-extract-plugin.

[alexander-akait]

@jamietre #202 should be fixed in plugin, no need hacks and new options for this. Better reproducible problem and fix it plugin.

Each new option increase complex program (we don't have enough contributors and time on option with rare use case) and better avoid this.

[jamietre]

@jamietre #202 should be fixed in plugin, no need hacks and new options for this. Better reproducible problem and fix it plugin.

I don't think you understand. This is not a bug, it will never be fixed because the behavior is expected. See:

#124 (comment)

@michael-ciniawsky
#246 But CSS Ordering is still not 💯 deterministic and likely never will be, you will get spammy warnings in case chunk1 and chunk2 imports modules in a different order, otherwise css-modules usage is encouraged to avoid issues wit ordering (e.g the CSS Cascade) altogether

This simply allows us to implement a solution to have deterministic CSS ordering in our app. There's probably no "right" answer for how to do this, and our implementation involves writing metadata (the lockfile) and some nontrivial logic, which is why it's out of scope for this plugin.

I don't think allowing end-users to have a hook to implement their own solution to CSS ordering is out of scope; but if it's not going to be allowed I'll either fork it or find another way.

[alexander-akait]

@michael-ciniawsky
#246 But CSS Ordering is still not 100 deterministic and likely never will be, you will get spammy warnings in case chunk1 and chunk2 imports modules in a different order, otherwise css-modules usage is encouraged to avoid issues wit ordering (e.g the CSS Cascade) altogether

Not sure it is truth, anyway if we need this it is should be as hook, not option.

Why hard to create minimum reproducible test repo? Maybe you case it is bug and should be fixed without hacks.

[jamietre]

@michael-ciniawsky
#246 But CSS Ordering is still not 100 deterministic and likely never will be, you will get spammy warnings in case chunk1 and chunk2 imports modules in a different order, otherwise css-modules usage is encouraged to avoid issues wit ordering (e.g the CSS Cascade) altogether

Not sure it is truth, anyway if we need this it is should be as hook, not option.

Why hard to create minimum reproducible test repo? Maybe you case it is bug and should be fixed without hacks.

This problem only manifests when a lot of complex CSS dependencies are involved and certain changes are made and/or when splitting out chunks. We experience it in our repo (with hundreds of CSS modules) from time to time. I can't publish our proprietary source code, nor am I even sure what steps I would have to take to reproduce it, since it doesn't happen often, so creating a repro case would be very time consuming.

I thought this was pretty well understood given the number of issues referencing non-deterministic CSS order, so apologies if that's not the case. But it would be a lot easier for me just to keep using the fork than to have to prove in an isolated repo that this problem exists.

And again - I'm not trying to solve any particular problem here but just give people an option to re-order CSS for whatever reason they need to. If there's a better way that would be acceptable (I don't know what you mean by hook) then I'm happy to revise.

[alexander-akait]

@jamietre

I thought this was pretty well understood given the number of issues referencing non-deterministic CSS order, so apologies if that's not the case. But it would be a lot easier for me just to keep using the fork than to have to prove in an isolated repo that this problem exists.

This is not the right solution. Maintaining the fork requires you extra time and cost. I do not need to prove the existence of a problem, I believe you. Why i need reproducible test repo? Because problem can't be located in plugin, maybe bug in chunks/modules order on webpack side or some loader output invalid order of require/import or maybe bug in you configuration, a lot of places where bug can be located and i want to fix this bug, create hacks is very bad approach. If problem on webpack side you can you can get this problem next time elsewhere. Will you also add options?

I do not ask to show the source code of the project, although I have already signed DNA several times for finding and fixing problems, so this is also not a problem.

Also as i write above we need hook, not option here, it is allow other people create plugins where they need other logic for modules.

[jamietre]

Again, there's no bug that needs to be fixed; we just want a way to re-order the CSS. Spending hours trying to create a repo that reproduces a problem that seems to be well understood is not a good use of my time. I'd rather not use a fork, but the cost is low compared to continuing this debate and doing a bunch of work to prove a problem exists, which isn't even directly related to the change I want, and which may not even result in this PR being accepted.

I'm not trying to push code that solves a particular problem so I don't see what bearing the repro of my particular need for this has, anyway. I'm just trying to give people a simple way to post-process the CSS that is produced to re-order it; there could be lots of applications for this.

Again I don't know what a "hook" is but if you'll accept this using some alternative implementation I'm glad to revise. You need to point me to the correct way, that's all.

Closing for now, if you change your mind let me know.

[alexander-akait]

@jamietre

Your communication violates the code of conduct, I get dozens of issues and PRs and I need time to understand the problem. I also need to discuss some aspects on implementation. It is normal for OSS. Unfortunately, your communication only indicates that I do not want to understand the problem and something to do with it, although this is not truth, so I have no desire to help you.

How implement hook and use this in own plugin https://webpack.js.org/api/plugins/#custom-hooks.

[jamietre]

@jamietre

Your communication violates the code of conduct, I get dozens of issue and PRs and I need time to understand the problem. I also need to discuss some aspects on implementation. It is normal for OSS. Unfortunately, your communication only indicates that I do not want to understand the problem and something to do with it, although this is not truth, so I have no desire to help you.

How implement hook and use this in own plugin https://webpack.js.org/api/plugins/#custom-hooks.

I'm really sorry; I've tried to be completely professional in my communications here. What did I say that violated the code of conduct?

I appreciate that you do not understand the problem; all I'm saying is that it would be a lot of effort for me to complete the tasks that you've asked of me here, and it's simply not worth it to me, since the alternative of using our fork is pretty low cost. This changes rarely, it it's easy to maintain our fork since the change is so trivial. I hope you can appreciate that I have to make decisions on how to use my time; while I'd rather get a change merged here that would enable us to not use a fork, it doesn't seem worth the cost at this point. I also appreciate that as a repo maintainer you have requirements for what code you'll accept, and I have no hard feelings if you don't want to accept this without my doing what you've asked. Are we cool?

[alexander-akait]

@jamietre yep, anyway please look on implementation as custom hook to allow other people write own logic. I just want to see you logic in onEmit , maybe this will allow me to understand the essence of the problem deeper.

[jamietre]

@jamietre yep, anyway please look on implementation as custom hook to allow other people write own logic. I just want to see you logic in onEmit , maybe this will allow me to understand the essence of the problem deeper.

Setting up a hook doesn't look to hard. I'm glad to revise this to use a hook if that gives us a good chance of getting it merged.

I thought about this some more and I realize I can demonstrate this issue in a pretty simple way. (This isn't actually exactly how it happens for us, since it's usually related to chunk splitting, but it's a contrived case)

Suppose you have files:

index1.js

import './a.css';
import './index2';
import './index3';

index2:

import './b.css';
import './c.css';

index3.js:

import './c.css';
import './b.css';

The output will be ordered as: a, b, c

If you revise index1:

index1.js

import './a.css';
import './index3';

The output is now ordered as a, c, b

This is completely expected; but it's not acceptable for us, because css conflicts between "b" and "c" could change the way content renders. Instead, we maintain a lock file that fixes the order "a, b, c" when those assets were first seen, so changing index1.js as I did would still result in the order being "a, b, c".

So of course ideally we wouldn't have CSS that conflicts in ways like this, but we have hundreds and hundreds of CSS modules and situations where specificity can alter the outcome. It's proved impossible to ensure it's all correct over time especially with shared code. This ensures that we don't have surprises.

[alexander-akait]

@jamietre thanks for the explanation, before answer i want to looks how other bundlers (parcel, rollup with css and other) resolve this problem, you can help me with this 👍 As minimum you should get warning on first example, because it is hard to say who really should be second/third (it is right?).

Also i think webpack already have hook what allow you to order modules as you want, need look in documentation (i.e. you can write simple plugin for this). Anyway need investigate how better we can solve this problem.

[jamietre]

@jamietre thanks for the explanation, before answer i want to looks how other bundlers (parcel, rollup with css and other) resolve this problem, you can help me with this 👍 As minimum you should get warning on first example, because it is hard to say who really should be second/third (it is right?).

This kind of conflict is pretty common when shared code is involved; if I have some small React component that's used in a lot of places, the position of it's CSS will depend entirely on when the first consumer is loaded. In a big application, that can easily change frequently. I'm not sure it should result in a warning, it's just the way we resolve CSS -- it goes in order of the first consumer. But we just want to make sure that order doesn't change once established.

Also i think webpack already have hook what allow you to order modules as you want, need look in documentation (i.e. you can write simple plugin for this). Anyway need investigate how better we can solve this problem.

I'm glad to solve this in whatever the easiest/most sensible way is. It's not obvious to me how I can change this after mini-css-extract-plugin is done though, since it transforms the CSS to imports/whatever and emits just the content back to Webpack. I don't think I have any options at that point other than reparsing that output, which now lacks the filenames, and would be very difficult anyway. It's entirely possible I'm missing something here, I haven't spent a lot of time in this part of the webpack pipeline, but I can't think how I'd be able to easily influence the final output at any point later than where I injected the hook here.

[alexander-akait]

Tomorrow I will evaluate all the possibilities and solutions and answer you, thanks for the feedback. Feel free to feedback and pinging me 👍

[jamietre]

Tomorrow I will evaluate all the possibilities and solutions and answer you, thanks for the feedback. Feel free to feedback and pinging me 👍

Thanks!

[codecov]

Codecov Report

Merging #322 into master will not change coverage.
The diff coverage is 0.00%.

@@          Coverage Diff           @@
##           master    #322   +/-   ##
======================================
  Coverage    0.00%   0.00%           
======================================
  Files           3       3           
  Lines         284     288    +4     
  Branches       59      61    +2     
======================================
- Misses        234     236    +2     
- Partials       50      52    +2     

Impacted Files	Coverage Δ
src/index.js	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffb0d87...034e778. Read the comment docs.