Audit function parameter and return types (2/2)

[lloydk]

Ensure that functions in the following modules have the correct types and ensure that all color types can be passed as parameters:

• inGamut
• luminance
• set
• setAll
• space
• toGamut

space.js

I wasn't sure about the types for the to and from methods on Colorspace. If we allow any color type and call getColor in those methods then we introduce a cyclic dependency between the getColor module and the space module. If we're ok having the cyclic dependency and want to allow any color type I can make the appropriate changes.

[netlify]

✅ Deploy Preview for colorjs ready!

Name	Link
🔨 Latest commit	bd5ede0
🔍 Latest deploy log	https://app.netlify.com/sites/colorjs/deploys/65de1909052f5700083042b9
😎 Deploy Preview	https://deploy-preview-457--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.

[lloydk]

I wasn't 100% convinced that getColor should be called in setAll but that's what I've gone with for now.

[jgerigmeyer]

I wasn't sure about the types for the to and from methods on Colorspace.

If the circular dependency doesn't hurt anything in practice, I'd lean toward calling getColor there to be consistent.

I wasn't 100% convinced that getColor should be called in setAll but that's what I've gone with for now.

I'm not opposed to calling getColor in set/setAll etc, but currently there are a number of types that are incorrect, some of them a bit tangential to this PR.

First, the type of getColor itself is wrong -- it claims to return a PlainColorObject (or PlainColorObject[]), where in reality if it is passed a Color object, it returns a Color object. That means that every fn which calls getColor and then returns the response (e.g. set, setAll, and toGamut) is also wrong -- they all claim to return a PlainColorObject but often return a Color.

Honestly, this (that is, keeping the types up-to-date with the codebase) is going to be an ongoing headache unless we're willing to start re-writing the code itself in TypeScript. That's something I'd be willing to consider taking on (potentially with help from @MysteryBlokHed, if he's willing), but I vaguely recall that @LeaVerou was opposed to that direction -- Lea, can you confirm?

[Edit: This TypeScript question should not hold up progress on this PR, and fixing the incorrect types.]

[lloydk]

I'm not opposed to calling getColor in set/setAll etc, but currently there are a number of types that are incorrect, some of them a bit tangential to this PR.

First, the type of getColor itself is wrong -- it claims to return a PlainColorObject (or PlainColorObject[]), where in reality if it is passed a Color object, it returns a Color object. That means that every fn which calls getColor and then returns the response (e.g. set, setAll, and toGamut) is also wrong -- they all claim to return a PlainColorObject but often return a Color.

Keep in mind that PlainColorObject is an interface and the Color type implements the PlainColorObject interface.

The types for the getColor function and other functions that return PlainColorObject simply guarantee that the object that is returned conforms to the PlainColorObject interface (space, coords and alpha properties). The object may be a Color, object literal, or some other class that has space, coords and alpha properties.

let x: PlainColorObject = getColor("red"); // returns an object literal
let y: PlainColorObject = getColor(new Color("red")); // returns a Color object
let z: Color = getColor(new Color("red")); // type error

If we want to make additional guarantees about returning the same object if it conforms to the PlainColorObject interface or that we may modify an object in place we can do that in the JSDoc comments.

None of the functions exported in 'colorjs.io/fn' should have types that reference the Color type because the Color class isn't available in colorjs.io/fn.

[LeaVerou]

Honestly, this (that is, keeping the types up-to-date with the codebase) is going to be an ongoing headache unless we're willing to start re-writing the code itself in TypeScript. That's something I'd be willing to consider taking on (potentially with help from @MysteryBlokHed, if he's willing), but I vaguely recall that @LeaVerou was opposed to that direction -- Lea, can you confirm?

I do have some reservations, one of which being that there are many folks who are proficient in color science but only dabble with JS, and a TS codebase is hostile to them. Whereas having the types separately means we can still accept contributions from folks who are not comfortable with TS, and adjust the typings ourselves after.

I’m willing to examine it , but could you elaborate a bit on how TS would help with this issue? I obviously get how it would help us keep types up to date, but not how it would help with the return type issue. My understanding is that TS does quite poorly with very dynamic APIs where functions are computed from other functions etc.

[jgerigmeyer]

Keep in mind that PlainColorObject is an interface and the Color type implements the PlainColorObject interface.

None of the functions exported in 'colorjs.io/fn' should have types that reference the Color type because the Color class isn't available in colorjs.io/fn.

@lloydk Thanks -- those are both really helpful points, and I was missing the forest for the trees. 🤦

I've pushed some proposed changes/additions for you to review in lloydk#1, mostly to see what it looks like if we enforce more consistent Color object return types from the Color namespace methods.

could you elaborate a bit on how TS would help with this issue? I obviously get how it would help us keep types up to date, but not how it would help with the return type issue.

@LeaVerou I'm distracting from the purpose of this PR, so I'll open a new issue to consider this question. 😄

My understanding is that TS does quite poorly with very dynamic APIs where functions are computed from other functions etc.

Yes, that can be true in some cases. I may try it out on a few methods and see what it looks like.

[jgerigmeyer]

I'm fine merging this as-is, then I have a follow-up PR I can open with a few more proposed changes.

[lloydk]

I'm fine merging this as-is, then I have a follow-up PR I can open with a few more proposed changes.

Did you still want me to review lloydk#1 or should I wait for the follow-up PR?

[jgerigmeyer]

I'm fine merging this as-is, then I have a follow-up PR I can open with a few more proposed changes.

Did you still want me to review lloydk#1 or should I wait for the follow-up PR?

Probably easier for me to open the PR here and review it then -- thanks!