Non-deterministic generated class/import name due to usage of importIndex

[OliverJAsh]

Hi,

We have been investigating an issue whereby generated class names would sometimes (but not always) change in-between builds, even when there are no changes in the source files. This is undesirable because it means generated assets will be cache busted for no good reason.

We have created a reduced test case which you can view and run here: https://github.com/Magellol/webpack-css-modules-keyframe-name-bug/tree/olly-postcss-modules-values-bug. The reduced test case consumes this module via css-loader (using the modules option).

• Clone the repository
• Run yarn
• Run ./test.sh. This script runs webpack multiple times. Each time it will check the output for any differences between the latest build and the previous build.

When we run this test, we will see that sometimes (but not always) there is a change in the output of webpack. Specifically, the hash of a generated class name changes:

diff --git a/test-2/main.fadd7.js b/test-3/main.395b7.js
similarity index 99%
rename from test-2/main.fadd7.js
rename to test-3/main.395b7.js
index e4660d2..f69e1f7 100644
--- a/test-2/main.fadd7.js
+++ b/test-3/main.395b7.js
@@ -17458,8 +17458,8 @@ exports.push([module.i, ".VJhuTjRiUDapSyp4Mz_WR {\n}", ""]);
 
 // Exports
 exports.locals = {
-	"animation": "_2c1o2cfFCMpnjxOIdv9qdH",
-	"i__const_animation_0": "_2c1o2cfFCMpnjxOIdv9qdH",
+	"animation": "_2A05uI8L01JWO8uhGPBJqU",
+	"i__const_animation_1": "_2A05uI8L01JWO8uhGPBJqU",
 	"topApplicationPlaceholder": "VJhuTjRiUDapSyp4Mz_WR " + __webpack_require__(0).locals["animation"] + ""
 };

We noticed this issue seems to go away after upgrading css-loader to from 2.1.1 to the immediate next version—3.0.0. As to why this seems to fix the issue, we think (but not certain) that it's something to do with the changes made in css-modules/postcss-modules-local-by-default#13 (released in version 2.0.6 of this module). (css-loader 3.0.0 depends on a newer version of the postcss-modules-local-by-default module which includes this change.)

However, whilst debugging this issue we noticed that this module (postcss-modules-values) uses a mutable variable (importIndex) to compute the "imported name", in createImportedName:

postcss-modules-values/src/index.js

Lines 11 to 15 in de45a53

	let importIndex = 0;
	let createImportedName =
	(options && options.createImportedName) ||
	((importName /*, path*/) =>
	`i__const_${importName.replace(/\W/g, '_')}_${importIndex++}`);

This means that createImportedName is not deterministic. When we run a build multiple times, the order that createImportedName is called for different imports may be different. Therefore it will return different results, because it relies on a mutable variable.

If we log the result of createImportedName in our reduced test case, we can see that the animation class produces different results between builds:

 let options = {};
 let importIndex = 0;
 let createImportedName =
   (options && options.createImportedName) ||
   ((importName /*, path*/) =>
     {
+      console.log({ importName, importIndex });
       return `i__const_${importName.replace(/\W/g, '_')}_${importIndex++}`;
     });

Sometimes the importIndex used in animation is 0:

{ importName: 'animation', importIndex: 0 }
{ importName: 'bg', importIndex: 1 }

Other times it's 1:

{ importName: 'bg', importIndex: 0 }
{ importName: 'animation', importIndex: 1 }

Despite the fact we were able to workaround this issue by a change in a separate module, I would like to ask why importIndex is used in this way? Is there a more reliable/deterministic value that can be used instead? It feels like using a mutable variable—scoped to the module—in this way could very easily lead to more bugs like the one I described above. The fact we were able to workaround by upgrading another module feels more like a coincidence than a real fix.

[alexander-akait]

Looks like bug, feel free to send a fix

[OliverJAsh]

@evilebottnawi I would be happy to contribute a fix but I'm not sure what to do. Maybe if you could answer these questions then I'll be able to work something out:

why importIndex is used in this way?

Why does the "imported name" need to contain the importIndex at all? What would happen if we just removed it?

Assuming there's a reason for the importIndex, we'll need to figure out a replacement.

[alexander-akait]

To be honestly I don't know, it was a long time ago, but I can look at this in near future, if you have time to investigate - feel free to do it

[alexander-akait]

Fixed (partial), now it is the option https://github.com/css-modules/postcss-modules-values/blob/master/src/index.js#L12 and generate unique id on the each plugin creation, you can specify more a predicted value based on filename, feel free to feedback