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

Fix mini-css-extract-plugin 0.4.3 issue #176

Closed
wants to merge 2 commits into from

Conversation

karlvr
Copy link

@karlvr karlvr commented Nov 13, 2018

Resolves #167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the module.userRequest is correct, and we add to moduleAssets correctly. However mini-css-extract-plugin then also reports the same file but with module.userRequest set to the CSS file that references it, which caused us to overwrite the good value in moduleAssets.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177

Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
@karlvr
Copy link
Author

karlvr commented Nov 13, 2018

Sorry, I have found a case where the first time the asset is reported it is from the CSS file, so this doesn't work.

@karlvr karlvr closed this Nov 13, 2018
Second attempt. Based on webdeveric/webpack-assets-manifest#40

Resolves shellscape#167

mini-css-extract-plugin reports additional, incorrect information for files that are refenced in CSS. The first time we see the file the `module.userRequest` is correct, and we add to `moduleAssets` correctly. However mini-css-extract-plugin then also reports the same `file` but with `module.userRequest` set to the CSS file that references it, which caused us to overwrite the good value in `moduleAssets`.

See the change in mini-css-extract-plugin that caused this webpack-contrib/mini-css-extract-plugin#177
@karlvr karlvr reopened this Nov 13, 2018
@karlvr
Copy link
Author

karlvr commented Nov 13, 2018

I've pushed a change which is based on the normalModuleLoader fix that webdeveric/webpack-assets-manifest#40 uses. That seems to consistently report the files in an order so that our check where we don't overwrite what we know about a file works.

@alexander-akait
Copy link
Contributor

/cc @mastilver

@fernandopasik
Copy link

would it be possible for someone to check this PR? @mastilver @danethurber ?

@alexander-akait
Copy link
Contributor

hm, we need:

  1. Tests here to avoid regressions
  2. Somebody can create minimum reproducible test repo? I should investigate this situation

@Lyrkan
Copy link

Lyrkan commented Dec 12, 2018

@evilebottnawi Here is a minimal example for your second point: https://github.com/Lyrkan/repro-manifest-plugin-167

@alexander-akait
Copy link
Contributor

alexander-akait commented Dec 12, 2018

WIP, confirmed.

i think it is bug in mini-css-webpack-plugin WIP

WIP on this inside webpack, please don't merge this, feedback soon

@fernandopasik
Copy link

@evilebottnawi is there a PR or issue in webpack to follow up this?

@alexander-akait
Copy link
Contributor

@fernandopasik sorry, a lot of developers on holidays, we can merge this as workaround right now and fix it late

@fernandopasik
Copy link

oh, that would be great!

@fernandopasik
Copy link

@evilebottnawi are there any updates on this one? :)

WIP on this inside webpack, please don't merge this, feedback soon

do you know which issue or pr is there to follow?

@alexander-akait
Copy link
Contributor

@fernandopasik can be merged as is, it will be solved in webpack@5 (need some refactor architecture)

@fernandopasik
Copy link

Oh good to know!
Could you merge this? Thanks for the hard work!!

@alexander-akait
Copy link
Contributor

@fernandopasik don't have permissions on this

/cc @mastilver

@fernandopasik
Copy link

@mastilver any chance we can merge this fix? webpack 5 is still far away from release

@alexander-akait
Copy link
Contributor

/cc @mastilver friendly ping again

@fernandopasik
Copy link

Hey @evilebottnawi, if @mastilver is not available do you think @danethurber could merge this?

@alexander-akait
Copy link
Contributor

I don't know, PR can be merged

@eugef
Copy link

eugef commented Jul 3, 2019

This fix works for me, any plans to merge the PR?

Meanwhile I decided to use instead https://github.com/webdeveric/webpack-assets-manifest

@alexander-akait
Copy link
Contributor

/cc @karlvr can you do rebase?

@eugef
Copy link

eugef commented Jul 3, 2019

@evilebottnawi as i can see this PR is already on top of the master branch, no rebase is needed

@alexander-akait
Copy link
Contributor

@eugef oh, yes, sorry

@shellscape
Copy link
Owner

Thanks for opening this PR a while back, and I'm sorry that it didn't get attention sooner. We've landed a major refactor in #222 that has resulted in some significant conflicts. If this issue is still outstanding, please open a new PR after rebasing to upstream/master and please make sure to include tests.

@shellscape shellscape closed this Oct 25, 2020
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

Successfully merging this pull request may close these issues.

Incompatibility with the latest version of mini-css-extract-plugin (0.4.3)
6 participants