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

TypeScript: args for color node only accept types matching second constructor overload for THREE.Color #581

Closed
neefrehman opened this issue Jul 19, 2020 · 13 comments

Comments

@neefrehman
Copy link

when trying to set the args prop for the <color /> node using a string or Color instance in TS, you get a type error.

the types for this in Three are correct, the complication is there are two constructors, the second one being an overload to allow for rgb args.

it looks like the NodeProps<Color> type only recognises this second overload and so will show an error if you pass any arguments that arent [number, number, number].

i've been searching through the type files on three and r3f but can't find any way for the NodeProps type to support both overloads.

image

image

@Fasani
Copy link
Member

Fasani commented Jul 20, 2020

I'll fix this.

@Fasani
Copy link
Member

Fasani commented Jul 21, 2020

Could you include more code as I do not seem to have the same issue? Did you make your own type for color?

@Fasani
Copy link
Member

Fasani commented Jul 21, 2020

@neefrehman
Copy link
Author

@Fasani no custom types! All I need to get the error is to use a string as args in the color node, like so:

<color attach="background" args={["#ffffff"]} />

Minimal demo with error here: codesandbox link

@drcmda
Copy link
Member

drcmda commented Jul 21, 2020

color in threejs is defined as

export class Color {
  constructor( color?: Color | string | number )
  constructor( r: number, g: number, b: number )

And we use ConstructorParameters to figure out arguments ...

type Args<T> = T extends new (...args: any) => any ? ConstructorParameters<T> : T

export interface NodeProps<T, P> {
  args?: Args<P>
  ...

export type Node<T, P> = Overwrite<Partial<T>, NodeProps<T, P>>

color: ReactThreeFiber.Node<THREE.Color, typeof THREE.Color>

so, does TS support constructor overloads in ConstructorParameters ?

EDIT:

doesn't look like it ... microsoft/TypeScript#37079

@neefrehman
Copy link
Author

neefrehman commented Jul 21, 2020

thats frustrating. I can't see any other overloaded constructors in three classes, so maybe not a broad enough issue for a fix? Using the suggested hack (shortened for only 2 overloads) in that issue would give us something like:

color: ReactThreeFiber.Node<ColorParams, typeof ColorParams>

...

type OrderedColorParams = OverloadedConstructorParameters<typeof THREE.Color>; // [[string | number | THREE.Color], [number, number, number]]
type ColorParams = OrderedColorParams[number]; // [string | number | THREE.Color] | [number, number, number]

type ConstructorOverloads<T> =
    T extends {
        new(...args: infer A1): infer R1; new(...args: infer A2): infer R2
    } ? [
        new (...args: A1) => R1, new (...args: A2) => R2
    ] : T extends {
        new(...args: infer A1): infer R1
    } ? [
        new (...args: A1) => R1
    ] : any

type OverloadedConstructorParameters<T> =
    ConstructorOverloads<T> extends infer O ?
    { [K in keyof O]: ConstructorParameters<Extract<O[K], new (...args: any) => any>> } : never

which looks to (kind of?) fix the args types, but gives rise to some array-type-related problems when using color that I can't make head or tails of:

image

image

@drcmda
Copy link
Member

drcmda commented Jul 21, 2020

it would be better to give color a customized definition. it looks to me like OverloadedConstructorParameters is quite a mess. can someone do that? i wouldn't know how ... but doing it directly where color is defined:

color: ReactThreeFiber.Node<THREE.Color, typeof THREE.Color> <---overwrite Args with constructorargs
                                                                 + overload for color | string | number

@neefrehman
Copy link
Author

i've just tried a few things to do this but nothing's worked yet. It's slightly out of my grasp i think

@drcmda drcmda closed this as completed Sep 26, 2020
@xih
Copy link

xih commented Jul 26, 2023

@neefrehman @drcmda Do we have a workaround for this typescript error?

Running into the same issue

@CodyJasonBennett
Copy link
Member

Which versions of R3F and @types/three are you using?

@xih
Copy link

xih commented Jul 26, 2023

@CodyJasonBennett

"@react-three/fiber": "^8.13.4",
"@types/three": "^0.153.0",

I'm just trying to fix this simple type error:
image

@CodyJasonBennett
Copy link
Member

You'll need to update to the latest version which includes a fix https://github.com/pmndrs/react-three-fiber/releases/tag/v8.13.5.

@xih
Copy link

xih commented Jul 26, 2023

@CodyJasonBennett

Sweet - that worked! thank you :))

bigmistqke added a commit to solidjs-community/solid-three that referenced this issue Aug 16, 2023
…nstructorParameters

`ConstructorParameters` does not include overloaded constructors
see microsoft/TypeScript#37079 and pmndrs/react-three-fiber#581
`r3f`s solution is to special-case Color in v8 and solve it later for v9, because they faced some problems with the solution/hack described in the typescript github-issue
The hack didn't seem to cause the same issues with us, so I implemented that instead.

It's a manual union of overloaded methods, going up to 7 overloads, which is probably a bit of overkill.
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

No branches or pull requests

5 participants