fix(index): assign empty module.id to prevent contenthash from changing unnecessarily

[lfre]

Content hashes change when the entries order or length changes

• Checks sourceMap before JSON.stringify, because it outputs "" on an empty string, modifying the hash unnecessarily.

Reproduction: https://github.com/lfre/mini-css-extract-plugin-contenthash

Type

• Fix

Issues

• Fixes Content hashes change based on entry order #281

SemVer

• Patch

[jsf-clabot]

All committers have signed the CLA.

[michael-ciniawsky]

@lfre Are those test also failing locally if you run them?

[lfre]

@michael-ciniawsky Yes, they are. It makes sense because this would change the hashes of the expected. I imagine I have to replace those. Do I go into each test and run their webpack configs, or is there another way?

[michael-ciniawsky]

Yeah, seems to be the case please double check if the file contents still match :) super annoying... 🙄

[michael-ciniawsky]

The async and MemoryFS test seem to be unrelated (not 💯 sure for the async one)

[lfre]

@michael-ciniawsky Updated the tests. A couple things to notice:

• The contents of 1.css and 2.css in composes-async are now flipped, meaning 1.css has the contents of https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/composes-async/expected/2.css
• In contenthash-multiple-entries a secondary file (https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/contenthash-multiple-entries/expected/7bb844e9f5514d392e40.css) with the same contents of https://github.com/webpack-contrib/mini-css-extract-plugin/blob/master/test/cases/contenthash-multiple-entries/expected/651d2818ea64cfe50dbe.css is no longer created.

Everything else maintains the same contents. Let me know if these are bugs or unexpected pluses. For the duped file in contenthash-multiple-entries seems like a fix. Ready for another look.

[lfre]

npm run test -- --runInBand passes locally, and I'm seeing cd0bb358c45b584743d8ce4991777c42.svg created locally in js/simple-publicpath, so I'm not sure what's up with that.

[lfre]

@michael-ciniawsky @evilebottnawi @sokra I'd appreciate some feedback here. 🙏

[sokra]

I wonder why this file was removed..

[lfre]

As far as I can tell, it's a duplicate of https://github.com/webpack-contrib/mini-css-extract-plugin/blob/860ba4abf6ca02b231108d1163749b334c2711d9/test/cases/contenthash-multiple-entries/expected/10adcc6fa8f8a3cf8d8d.css.

[sokra]

👍

[alexander-akait]

Looks tests also broken 😕

[lfre]

@evilebottnawi Tests are passing locally using latest node 8. Only one test is failing if you see the log and my comment #284 (comment). I'm going to try downgrading to node 6 to test but I'd appreciate any other ideas on how to approach this.

[alexander-akait]

/cc @sokra can we merge and release patch?

[sokra]

/cc @sokra can we merge and release patch?

👍

[michael-ciniawsky]

Released in v0.4.4 🎉

[erykpiast]

@lfre @michael-ciniawsky I have a question about this change or module id in general. In such case, when module is somehow empty, NamedModulesPlugin creates name like ?abcdef. For regular ones (or Normal in webpack's nomenclature) it outputs something like ./foo/bar/baz.js?abcdef. Until that ?... hash is based on FULL request path (ex. /home/user/projects/foo/bar/baz.js), it's different for different repository clones, ex. on different workstations and on CI. It makes file hashes quite unpredictable as sorting modules by id gives different results on different machines and hash calculation depends on that order (hash.update('a') && hash.update('b') gives different results than hash.update('b') && hash.update('a')).

So, my question is: would it be an issue to initialize id of CssModule with something like ${dependency.id}?css? I mean some value that starts with normal module id, so module order would be stable and not really dependent on that weird hash created from full request path.

[lfre]

@erykpiast Yes, initially I suggested using dependency.context #281 (comment), but id would also work. If you're using CI for your deployments, this wouldn't be a problem because the path would always be the same, but I can see this being an issue for manual uploads from different workstations.

[erykpiast]

Hey @lfre, thank you for the response! I cannot get why your solution wasn’t accepted, what was the issue with it?

In my case CI is an issue as well, repository is cloned to directory prefixed with build ID, so absolute path differs from build to build. In the worst case, file hash changes when its contents did not change, so the whole idea of hashing looses its sense.

[erykpiast]

@lfre Maybe I'll open separate issue for this?

[lfre]

Go for it @erykpiast

[erykpiast]

👍#308