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

Color space specific accessors do not work in typescript #381

Closed
micobarac opened this issue Dec 22, 2023 · 7 comments · Fixed by #389
Closed

Color space specific accessors do not work in typescript #381

micobarac opened this issue Dec 22, 2023 · 7 comments · Fixed by #389
Assignees

Comments

@micobarac
Copy link

Coordinates of the color's color space are available without a prefix:

let color = new Color("slategray").to("lch");
color.l = 80; // Set LCH lightness
color.c *= 1.2; // saturate by increasing LCH chroma

This triggers an error:

Property 'l' does not exist on type 'Color'.

@micobarac micobarac changed the title Colorspace specific accessors do not work in typescript Color space specific accessors do not work in typescript Dec 22, 2023
@MysteryBlokHed
Copy link
Member

The best idea I can think of is to change the Color class to have a property like [coord: string]: number, so that any property that isn't explicitly defined is just a number. The only unfortunate thing about that is that type checking would essentially be off for coordinate properties, meaning that TypeScript won't catch things like typos on those properties.

The property on Color could also be [coord: string]: number | undefined instead, meaning that TypeScript won't falsely guarantee that the properties will be numbers. But if that's used, then a non-null assertion would have to be used in some cases, which could get annoying:

color.c! *= 1.2;
//     ^ non-null assertion required, since color.c must be defined to multiply it

That also doesn't change the fact that something like this would be allowed:

color.foobar = 123; // allowed, since 123 satisfies `number | undefined`

@jgerigmeyer
Copy link
Member

@MysteryBlokHed Since the SpaceAccessor definition already uses number and allows any key (e.g. color.lch.foobar works), I'd lean toward your first suggestion here to be consistent with that: [coord: string]: number.

However, since TypeScript classes can't have index signatures, I'm honestly not sure how to implement this on Color without explicitly defining all the supported channel names (like we're doing with all the supported space names). Do you have a suggestion for how to get around that?

@MysteryBlokHed
Copy link
Member

Right, I forgot about that. I'm not sure that there's a good way to get around that, so the best option might just be to manually add a property for every possible space coordinate.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 6, 2024

Right, I forgot about that. I'm not sure that there's a good way to get around that, so the best option might just be to manually add a property for every possible space coordinate.

Surely that can't be the only way? How do you describe an object that is a proxy and can truly accept arbitrary properties then?

@MysteryBlokHed
Copy link
Member

Usually you could use something like this, which would allow any key of type string to be given a value of type number:

interface Example {
    [key: string]: number;
}

The problem is that the types of all other properties in the object also have to fit in that type.

If you try to add something like [coord: string]: number to the Color class, then TypeScript complains because all the methods and properties of the class aren't assignable to the type number.

As far as I can tell, this is a deliberate choice by the TypeScript team and there isn't a good way around it. I'd definitely prefer to do it like this since it's a lot easier to maintain (i.e. no need to change the type definitions if new coordinate names are ever required), but I don't know if that's possible.

@LeaVerou
Copy link
Member

LeaVerou commented Jan 7, 2024

I see. If we do have to explicitly list all color spaces and coordinate names, I think it's a much better strategy to do it via a build script rather than having to manually keep things in sync. But even then, it kinda breaks the whole extensibility argument — how would someone else adding an ad hoc color space extend the definition?

@LeaVerou
Copy link
Member

LeaVerou commented Jan 7, 2024

I wonder if there would be value in posting about our use case to the TS repo so they could be aware of use cases like this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants