Color constructor should handle argument as immutable

[bennyschudel]

Hey there!

I was surprised to figure out that a call to new Color({ ... }) modifies my provided object by adding the space to it.

I guess this is a 🪲

Best
Benny

[LeaVerou]

Ah yeah, it should copy the object, at least when it needs to be modified. Especially since perf is not a huge consideration with the OOP API.

[MysteryBlokHed]

It looks like getColor modifies the passed object, adding the space key:

color.js/src/getColor.js

Lines 40 to 43 in 45ba172

	if (!(space instanceof ColorSpace)) {
	// Convert string id to color space object
	color.space = ColorSpace.get(space);
	}

This could be solved by calling structuredClone before getColor in the Color constructor:

color.js/src/color.js

Lines 48 to 56 in 45ba172

	if (args.length === 1) {
	let parseMeta = {};
	color = getColor(args[0], {parseMeta});
	if (parseMeta.format) {
	// Color actually came from a string
	this.parseMeta = parseMeta;
	}
	}

i.e.:

color = getColor(structuredClone(args[0]), {parseMeta});

[LeaVerou]

Not sure we need structuredClone(), as this is a flat object, so Object.assign({}, ...) should work?

[MysteryBlokHed]

Object.assign wouldn't work on Color class instances that get passed afaik, which is why I suggested structuredClone. But after looking at the code, I don't think there's actually a way for a Color instance to have its space property mutated by getColor (since the function only adds the space property if it is missing, and Color instances always have that property). So we could probably just do Object.assign if the passed argument is an object, and leave other inputs alone?

[LeaVerou]

We can detect if something is a plan object and branch accordingly. Not sure if that would be the best course of action, just throwing it out there as an option.

[MysteryBlokHed]

That seems like the most performant thing to do—although I don't actually know the best way to check if something is a plain object in JS. Maybe args[0].constructor === Object? Or we could also just check if it's not an instance of Color.

[LeaVerou]

That seems like the most performant thing to do—although I don't actually know the best way to check if something is a plain object in JS. Maybe args[0].constructor === Object? Or we could also just check if it's not an instance of Color.

This is how I did it in the past: https://github.com/mavoweb/mavo/blob/main/src/util.js#L61 ($.type() just checks Object.protoyype.toString.call(), we must have a similar util here)

[MysteryBlokHed]

Based on that, I think something like typeof args[0] === 'object' && Object.getPrototypeOf(args[0]).constructor === Object should work.
I should be able to open a PR for the change this evening