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

Moved function overloads from Color.set to rest types #615

Merged
merged 2 commits into from
Sep 28, 2023

Conversation

grischaerbe
Copy link
Contributor

@grischaerbe grischaerbe commented Sep 27, 2023

Moved function overloads from Color.set to rest types in order for Parameters<Color['set']> to pick it up correctly.

Why

Color.set is implemented as a function with two overloads. The TypeScript utiliity type Parameters<T> will only return the parameter types of the last overload. Threlte (and other frameworks such as @react-three/fiber) is using this type to infer the possible prop types, for example:

<T.MeshStandardMaterial color="red" />
<T.MeshStandardMaterial color={[0.5, 1, 0.5]} />

What

A possible solution is to implement the types as rest types. This PR is marked as a draft PR for discussion purposes. If this is a viable way to move forward, I would be happy to remove the draft status.

Checklist

  • Checked the target branch (current goes master, next goes dev)
  • Ready to be merged

…arameters<T>` utility type to pick it up correctly
@Methuselah96
Copy link
Contributor

Thanks for the PR!

In general, I prefer to keep workarounds as close to the source of the problem as possible. In this case, we're compensating for the shortcomings of the Parameters<T> type. I'm curious if any of the workarounds in microsoft/TypeScript#32164 like microsoft/TypeScript#32164 (comment) could be used for this?

@grischaerbe
Copy link
Contributor Author

grischaerbe commented Sep 28, 2023

Hi @Methuselah96,

Thanks for sharing the workarounds. I created a TypeScript playground to show that the solution proposed in this PR is as safe as the current solution and also provides intellisense as function overloads. On top of that Parameters<Color['set']> (rest in this example) would resolve all parameters, which is beneficial to all type consumers.
The reason why I refrain from the workaround is that we would need to apply that to not only Color['set'] but essentially all props, I guess we'd run into edge cases with this hard to maintain type in no time.

@Methuselah96
Copy link
Contributor

Yeah, makes sense, I assume we would only be doing this for any overloaded set functions?

I'm probably okay with this as long as this pattern doesn't need be to used too much, and with the understanding if any downsides emerge down the road that we're likely to go back to using overloads.

@grischaerbe
Copy link
Contributor Author

Yeah, makes sense, I assume we would only be doing this for any overloaded set functions?

That would make sense, although I'm not aware of other ones. At least this is the one with the largest impact on our user side.

I'm probably okay with this as long as this pattern doesn't need be to used too much, and with the understanding if any downsides emerge down the road that we're likely to go back to using overloads.

Absolutely. If any downsides emerge, I'm all for reverting.

Thanks for considering! I'll mark this ready for review then.

@grischaerbe grischaerbe marked this pull request as ready for review September 28, 2023 11:26
@Methuselah96 Methuselah96 changed the base branch from master to dev September 28, 2023 12:22
@Methuselah96 Methuselah96 merged commit 54f4edc into three-types:dev Sep 28, 2023
3 checks passed
@Methuselah96
Copy link
Contributor

Thanks! This will get released with r157 at the end of the month.

@Methuselah96
Copy link
Contributor

cc @CodyJasonBennett This might be useful for @react-three/fiber, but I guess can't be depended on until R3F requires @types/three@0.157.0 or later.

@CodyJasonBennett
Copy link
Contributor

Thanks for looking out. Yeah, we worked around this already, but I'm happy to review any related work in the future.

@grischaerbe
Copy link
Contributor Author

Hey @CodyJasonBennett, sorry, didn't want to blindside you. Is there a blocker for this from the R3F side? I guess this is also beneficial for you.

@CodyJasonBennett
Copy link
Contributor

No blocker, I've worked around this in pmndrs/react-three-fiber#2931 and pmndrs/react-three-fiber#2932 (the v9 branch may be easier to follow for further inspection). We rely heavily on inference, moreso in v9 also which is strictly for types/internal tests.

The linked workarounds from TS triage don't work very well since they can create complex unions that fail from complexity. What you can do alternatively to this PR, while still providing overloads, is follow overloads with a mixed signature that reflects the implementation:

set(r: number, g: number, b: number): this;
set(color: ColorRepresentation): this;
set(...args: [color: ColorRepresentation] | [r: number, g: number, b: number]): this;

Parameters always takes the last signature, so this should still play nice if this would be preferable to author. This goes for constructor signatures also.

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 this pull request may close these issues.

3 participants