-
Notifications
You must be signed in to change notification settings - Fork 82
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
Add types for color space coord accessors. #389
Conversation
* main: Fix toPrecition (was off by one for fractional inputs) (color-js#384) Add the HCT color space (color-js#380) Add gamutSpace to ColorSpace Add CJS type defs for node16 resolution. (color-js#383)
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@LeaVerou @MysteryBlokHed What do you think of this approach for fixing #381? It adds a build step (run during Note that users can still add their own spaces and augment the types by adding an e.g. import 'colorjs.io';
declare module 'colorjs.io' {
export default interface Color {
my-coord: number;
}
} |
Oof, this is tough. It’s not just about authors adding color spaces, but also about using a subset of Color.js with fewer color spaces. Is this the only solution? What if we simply forgo type checking for that part of the PI and write the definition so that it assumes that any unrecognized property access is a color space / coord accessor (respectively)? |
@LeaVerou I'm not sure how this relates to users using a subset with fewer color spaces. That is, there isn't a good way for TypeScript to know which color spaces a user has registered, so this allows coord accessors for all the known color spaces, but errors if a user tries to access an unknown coord. Similarly, we allow e.g.
That's what we initially discussed in #381, but TypeScript doesn't allow us to type specific methods/properties on Actually, I think I'd propose we automate the space types similar to these coord accessors -- I can push that change in a few minutes. |
In the sense that TS won't complain if the user is trying to use a color space they have not registered, but I guess that's ok?
Is this a bug in TS, or an intentional design decision? Regardless of what we do, I still think it would be useful to bring up this use case with the TS team and see if they want to address it. As far as this PR is concerned, I guess anything is better than the current situation, so I'm inclined to say let's merge (if @MysteryBlokHed is also on board). |
I think the problem of allowing unregistered space coordinates to be changed is alright, since it shouldn't actually cause any runtime errors—it just won't actually affect anything. Since TypeScript unfortunately doesn't allow us to just add a default type for undefined properties, this seems to me like the best solution. |
Fixes #381.