Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Nov 3, 2024
4 changes: 2 additions & 2 deletions src/ColorSpace.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* Defines the class and other types related to creating color spaces.
* For the builtin color spaces, see the `spaces` module.
*/
import { type, isNone } from "./util.js";
import { type, isNone, isInstance } from "./util.js";
import Format from "./Format.js";
import {getWhite} from "./adapt.js";
import hooks from "./hooks.js";
Expand Down Expand Up @@ -294,7 +294,7 @@ export default class ColorSpace {
* @param {ColorSpace | string} name
*/
static get (space, ...alternatives) {
if (!space || space instanceof ColorSpace) {
if (!space || isInstance(space, this)) {
return space;
}

Expand Down
5 changes: 3 additions & 2 deletions src/Format.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { isNone } from "./util.js";
import { isInstance, isNone } from "./util.js";
import Type from "./Type.js";

// Type "imports"
Expand Down Expand Up @@ -131,13 +131,14 @@ export default class Format {
return this.type === "function" || /** @type {any} */ (this).serialize;
}


/**
* @param {Format | FormatInterface} format
* @param {RemoveFirstElement<ConstructorParameters<typeof Format>>} args
* @returns {Format}
*/
static get (format, ...args) {
if (!format || format instanceof Format) {
if (!format || isInstance(format, this)) {
return /** @type {Format} */ (format);
}

Expand Down
4 changes: 2 additions & 2 deletions src/Type.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { serializeNumber, mapRange } from "./util.js";
import { serializeNumber, mapRange, isInstance } from "./util.js";

export default class Type {
// Class properties - declared here so that type inference works
Expand Down Expand Up @@ -117,10 +117,10 @@
}

static get (type, ...args) {
if (type instanceof this) {
if (isInstance(type, this)) {
return type;
}

return new this(type, ...args);

Check failure on line 124 in src/Type.js

View workflow job for this annotation

GitHub Actions / Lint & Test Types

A spread argument must either have a tuple type or be passed to a rest parameter.
}
}
2 changes: 1 addition & 1 deletion src/color.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,7 @@ export default class Color {
* Basically gets us the same result as new Color(color) but doesn't clone an existing color object
*/
static get (color, ...args) {
if (color instanceof Color) {
if (util.isInstance(color, this)) {
return color;
}

Expand Down
4 changes: 2 additions & 2 deletions src/getColor.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import ColorSpace from "./ColorSpace.js";
import {isString} from "./util.js";
import {isString, isInstance} from "./util.js";
import parse from "./parse.js";

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

if (!(space instanceof ColorSpace)) {
if (!isInstance(space, ColorSpace)) {
// Convert string id to color space object
color.space = ColorSpace.get(space);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed in 0123830

}
Expand Down
31 changes: 31 additions & 0 deletions src/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -172,3 +172,34 @@ export function bisectLeft (arr, value, lo = 0, hi = arr.length) {
}
return lo;
}

/**
* Determines whether an argument is an instance of a constructor, including subclasses.
* This is done by first just checking `instanceof`,
* and then comparing the string names of the constructors if that fails.
* @param {any} arg
* @param {C} constructor
* @template {new (...args: any) => any} C
* @returns {arg is InstanceType<C>}
*/
export function isInstance (arg, constructor) {
if (arg instanceof constructor) {
return true;
}

const targetName = constructor.name;

while (arg) {
const proto = Object.getPrototypeOf(arg);
const constructorName = proto?.constructor?.name;
if (constructorName === targetName) {
return true;
}
if (!constructorName || constructorName === "Object") {
return false;
}
arg = proto;
}

return false;
}
9 changes: 9 additions & 0 deletions types/test/util.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
interpolate,
interpolateInv,
mapRange,
isInstance,
} from "colorjs.io/src/util";

isString("foo"); // $ExpectType boolean
Expand Down Expand Up @@ -46,3 +47,11 @@ mapRange([1, 2], [3, 4]);
mapRange([1, 2, 3], [4, 5, 6], 7);

mapRange([1, 2], [3, 4], 5); // $ExpectType number

class SomeClass {}

declare const instance: any;

if (isInstance(instance, SomeClass)) {
instance; // $ExpectType SomeClass
}
Loading