Replace `instanceof` checks to work across color.js sources #605

MysteryBlokHed · 2024-10-28T13:25:22Z

Closes #602

Instead of relying solely on instanceof to check if an argument is an instance of some class (eg. Color or ColorSpace), also checks the name of the constructors of the instance to see if any of them have the same name as the target class.

This method isn't perfect, but it should work well enough given that it seems unlikely for a user of color.js to define their own unrelated class whose name conflicts with the library.

I'm open to changing this way of checking inheritance if anybody has better ideas.

Instead of relying solely on `instanceof` to check if an argument is an instance of some class (eg. Color or ColorSpace), also checks the name of the constructors of the instance to see if any of them have the same name as the target class. This method isn't perfect, but it should work well enough given that it seems unlikely for a user of color.js to define their own unrelated class whose name conflicts with the library.

netlify · 2024-10-28T13:25:39Z

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	`b43cf4e`
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/67280523a656e30008d1129c
😎 Deploy Preview	https://deploy-preview-605--colorjs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Forgot to remove this after debugging.

LeaVerou

Hey, thanks so much for working on this!

I love the idea of a generic isInstance() factory! Do we still need the public-facing APIs? Not sure we have authors that need to check if an object is a ColorSpace or a Format (I'd wager in most cases ColorSpace.get() would suffice). It seems more useful for Color since it does more than the instance check, though even there a static isColor() could be misconstrued as to also account for parseable color strings.

Since the API needs some more discussion, can we skip the public-facing APIs for now and just use private consts in each corresponding module? It seems in most cases the instanceof check is in the same module as the class it's checking, with one exception that can actually be refactored to a string check.

LeaVerou · 2024-10-28T16:15:41Z

src/getColor.js

@@ -37,7 +37,7 @@ export default function getColor (color, options) {
 	// Object fixup
 	let space = color.space || color.spaceId;

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


We could actually refactor this check to check if space is a string.

the util.isString() is "slow" and came up as a hotspot when I was running the texel benchmarks so I don't think we should add an additional call to isString().

Oh interesting. Maybe just typeof space === "string" then?

Changed in 0123830

LeaVerou · 2024-10-28T16:23:48Z

Or actually — not use a const at all. Instead of isInstance() being a factory forcing people to call it in two steps, we could make that an implementation detail by using a WeakMap to associate constructors with their isInstance functions and every time it's called, we could check if we have already generated a function for this constructor, and if not, generate it and store it in the cache. Or, a regular object literal that keys the constructor name to the function.

Though wait a second, why are we generating functions at all? There is no one-time work that we are saving that way, it's literally function currying, that users of this function can do with function.bind().

lloydk · 2024-10-28T16:19:25Z

src/color.js

+	static isColorInstance = util.isInstance(this);
+
+	static isColor (arg) {
+		return Color.isColorInstance(arg) || Array.isArray(arg?.coords);


I think the second part of the check should test to see if arg conforms to the PlainColorObject interface.

lloydk · 2024-10-28T16:23:51Z

src/color.d.ts

@@ -159,6 +159,10 @@ declare class Color extends SpaceAccessors implements PlainColorObject {
 		| Record<string, DefineFunctionHybrid>
 	): void;

+	static isColorInstance (arg: any): arg is Color;
+
+	static isColor (arg: any): arg is (Color | { coords: Coords });


Suggested change

static isColor (arg: any): arg is (Color | { coords: Coords });

static isColor (arg: any): arg is (Color | PlainColorObject);

lloydk · 2024-10-28T16:40:19Z

I love the idea of a generic isInstance() factory! Do we still need the public-facing APIs? Not sure we have authors that need to check if an object is a ColorSpace or a Format (I'd wager in most cases ColorSpace.get() would suffice). It seems more useful for Color since it does more than the instance check, though even there a static isColor() could be misconstrued as to also account for parseable color strings.

I'm not convinced we need the static isColor() function mostly for the reason you mentioned.

If we think it may be useful then I think we should add a standalone function that checks to see if an object conforms to the PlainColorObject interface so that it can be used from the procedural API. Color.isColor() can then do the isInstance() check and call the new standalone function.

MysteryBlokHed · 2024-10-28T20:59:34Z

Though wait a second, why are we generating functions at all? There is no one-time work that we are saving that way, it's literally function currying, that users of this function can do with function.bind().

I wrote isInstance that way just as a convenience when adding the static methods for other classes—I didn't mean for it to be part of the public API that library consumers would use, since I figure it would be more intuitive for developers to call eg. ColorSpace.isColorSpace rather than something like isInstance(arg, ColorSpace).

But if it's unlikely that developers would need to use these static methods, then I'd be fine with rewriting isInstance to just be a regular function with two parameters.

LeaVerou · 2024-10-29T18:06:19Z

Yeah, I think for now let's punt the static methods and just use IsInstance() directly.

lloydk

LGTM

Checks if the space is a string instead of checking if it's not a `ColorSpace` instance.

LeaVerou · 2024-11-03T05:01:45Z

Is this ready for a re-review? Also, could you please add an entry to the changelog in the same PR?

MysteryBlokHed · 2024-11-03T11:38:22Z

Is this ready for a re-review?

Should be ready now

Also, could you please add an entry to the changelog in the same PR?

Done!

releases/v0.6.0.md

LeaVerou

LGTM, see comments on changelog. Thank you!

Co-authored-by: Lea Verou <lea@verou.me>

MysteryBlokHed requested review from lloydk and LeaVerou October 28, 2024 13:25

fix: Remove break in isInstance function

817a2ac

Forgot to remove this after debugging.

LeaVerou requested changes Oct 28, 2024

View reviewed changes

lloydk requested changes Oct 28, 2024

View reviewed changes

MysteryBlokHed added 6 commits October 31, 2024 09:14

Remove static is functions and change isInstance signature

ec0f780

Update JSDoc description for isInstance

6b52450

[types] Remove obsolete ColorSpace test

9172833

[types] Remove obsolete Color test

f7dc8e6

[types] Add direct isInstance tests

6c8eea7

Use isInstance in getColor.js

59d1f22

MysteryBlokHed requested review from lloydk and LeaVerou October 31, 2024 18:35

lloydk approved these changes Oct 31, 2024

View reviewed changes

Update getColor space check

0123830

Checks if the space is a string instead of checking if it's not a `ColorSpace` instance.

Mention #605 in changelog

51cca5b

LeaVerou reviewed Nov 3, 2024

View reviewed changes

releases/v0.6.0.md Outdated Show resolved Hide resolved

LeaVerou approved these changes Nov 3, 2024

View reviewed changes

Add author @ to changelog

b43cf4e

Co-authored-by: Lea Verou <lea@verou.me>

MysteryBlokHed merged commit 5c1536e into main Nov 3, 2024
5 checks passed

MysteryBlokHed deleted the replace-instanceof branch November 3, 2024 23:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace `instanceof` checks to work across color.js sources #605

Replace `instanceof` checks to work across color.js sources #605

MysteryBlokHed commented Oct 28, 2024

netlify bot commented Oct 28, 2024 •

edited

Loading

LeaVerou left a comment

LeaVerou Oct 28, 2024

lloydk Oct 31, 2024

LeaVerou Nov 1, 2024

MysteryBlokHed Nov 3, 2024

LeaVerou commented Oct 28, 2024

lloydk Oct 28, 2024

lloydk Oct 28, 2024

lloydk commented Oct 28, 2024

MysteryBlokHed commented Oct 28, 2024 •

edited

Loading

LeaVerou commented Oct 29, 2024

lloydk left a comment

LeaVerou commented Nov 3, 2024

MysteryBlokHed commented Nov 3, 2024

LeaVerou left a comment •

edited

Loading

	static isColor (arg: any): arg is (Color \| { coords: Coords });
	static isColor (arg: any): arg is (Color \| PlainColorObject);

Replace instanceof checks to work across color.js sources #605

Replace instanceof checks to work across color.js sources #605

Conversation

MysteryBlokHed commented Oct 28, 2024

netlify bot commented Oct 28, 2024 • edited Loading

✅ Deploy Preview for colorjs ready!

LeaVerou left a comment

Choose a reason for hiding this comment

LeaVerou Oct 28, 2024

Choose a reason for hiding this comment

lloydk Oct 31, 2024

Choose a reason for hiding this comment

LeaVerou Nov 1, 2024

Choose a reason for hiding this comment

MysteryBlokHed Nov 3, 2024

Choose a reason for hiding this comment

LeaVerou commented Oct 28, 2024

lloydk Oct 28, 2024

Choose a reason for hiding this comment

lloydk Oct 28, 2024

Choose a reason for hiding this comment

lloydk commented Oct 28, 2024

MysteryBlokHed commented Oct 28, 2024 • edited Loading

LeaVerou commented Oct 29, 2024

lloydk left a comment

Choose a reason for hiding this comment

LeaVerou commented Nov 3, 2024

MysteryBlokHed commented Nov 3, 2024

LeaVerou left a comment • edited Loading

Choose a reason for hiding this comment

Replace `instanceof` checks to work across color.js sources #605

Replace `instanceof` checks to work across color.js sources #605

netlify bot commented Oct 28, 2024 •

edited

Loading

MysteryBlokHed commented Oct 28, 2024 •

edited

Loading

LeaVerou left a comment •

edited

Loading