Fix circular reference

[facelessuser]

util.js should not require Color from color.js which requires util.js
This breaks esm packages, and probably other as well.

Fixes #137

[facelessuser]

Let me know if you want it resolved differently. I went for the most obvious path.

[LeaVerou]

Hey, thank you for working on this!

I'm trying to figure out what the problem is, since color.js works fine when the browser resolves the dependency tree. Is this a bug in Rollup?

[facelessuser]

@LeaVerou I'll explain this as best I can because I'm not quite sure how your site loads up the lib and from which of the various builds it uses, so I'll speak specifically on the esm issue as you should be able to easily inspect this locally and see what I am referring to.

Description of Issue

Without this patch, you can look at the compiled esm module and inspect the source. util.js does not include color.js, but references Color. While rollup is building this, because util.js does not import color.js, it thinks that the Color in util.js is different than the one in color.js.

So, when Rollup compiles it all into one package, the util.js reference gets named Color (probably because it is resolved first in the package) and the actual Color object gets named as Color$1:

function parseCoord(coord) {
	if (coord.indexOf(".") > 0) {
		// Reduce a coordinate of a certain color space until the color is in gamut
		let [spaceId, coordName] = coord.split(".");
		let space = Color.space(spaceId);

		if (!(coordName in space.coords)) {
			throw new ReferenceError(`Color space "${space.name}" has no "${coordName}" coordinate.`);
		}

		return [space, coordName];
	}
}

class Color$1 {
	// Signatures:
	// new Color(stringToParse)
	// new Color(otherColor)
	// new Color(coords, alpha) // defaults to sRGB
	// new Color(CSS variable [, root])
	constructor (...args) {
		let str, color;

		// new Color(color)
		// new Color({spaceId, coords})
		// new Color({space, coords})
		if (args[0] && typeof args[0] === "object" && (args[0].space || args[0].spaceId) && args[0].coords) {
			color = args[0];
		}

This is why Color is not defined, the Color object is now compiled as Color$1.

Verification

I was able to verify this behavior locally first by inspection, and then I created a simple HTML file that included the local esm file (disable CORS so I could test it), and the old esm file would throw an error in the console due to this undefined Color reference.

After applying the patch, and doing the same as above, I was able to verify the issue was resolved. And I was able to inspect the esm module and see that it was, indeed, using the exact same reference now.

Possible Resolution

1. Have util.js import color.js. This is a bit 🤮 as we are creating a circular dependency. You will get warnings in the build about this, but this could work, I believe. This was not my first choice, but IIRC, it does work and is a viable solution.
2. Or, as I have done, you can move parseCoords into color.js, the only place it is used, and it is only used once. I did not see it documented publicly either, so it doesn't affect the "publicly" documented API.
3. I assume the logic could be absorbed directly into the function that calls it. It is literally only called in one place.

There may be other solutions. My curiosity got the better of me which is why I dug a bit deeper. I was able to at least isolate the issue quicker than I thought. I really only considered the three above options and then settled on number two, though maybe three would be cleaner.

I figured there was a good possibility that there would be a discussion on this, so I didn't worry about the "best solution" as much as presenting "a solution". I figured it was possible you had some other ideas on how to approach it. At the very least, this PR makes the root of the issue well known 🙂.

[facelessuser]

I actually found a way that is probably preferable. The code can actually remain the same, you just need to import color,js into util.js and ignore the circular dependency. Then it all resolves as expected.

It seems to be the way to work around this behavior of rollup. For more info, see here: rollup/rollup#1089.

[facelessuser]

I guess I should've mentioned, I've updated the pull request to utilize the circular import.

[LeaVerou]

Thank you so much! Merging