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

[v9] fix(useLoader): properly infer Loader types #2723

Merged
merged 12 commits into from
Jun 22, 2023
Merged

Conversation

CodyJasonBennett
Copy link
Member

This may need to be backported to master (#2341), but addresses a useLoader regression described in #2668 (comment).

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jan 19, 2023

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit b22c406:

Sandbox Source
example Configuration

@krispya
Copy link
Member

krispya commented Feb 27, 2023

I fixed this locally by extending all string | string [] to string | string[] | string[][]. The reason is that CubeTextureLoader inside of useLoader takes and array of an array of strings: #602

@CodyJasonBennett
Copy link
Member Author

So this would be url: string | string[] | string[][]? That's a bit unfortunate, but good if TS picks it up.

@krispya
Copy link
Member

krispya commented Feb 27, 2023

Also, useLoader previously took 2 type arguments ie useLoader<GLTF, T> and now takes 3-4. Is that expected? Otherwise all type issues with useLoader are resolved with these changes.

@CodyJasonBennett
Copy link
Member Author

CodyJasonBennett commented Feb 27, 2023

The latter 2 are not supposed to be public, that was an oversight from #2598. Can use typeof Proto and the fourth is redundant.

@krispya
Copy link
Member

krispya commented Feb 27, 2023

So this would be url: string | string[] | string[][]? That's a bit unfortunate, but good if TS picks it up.

It ends up looking this, including for useLoader:

export interface Loader<T> extends THREE.Loader {
  load(
    url: string | string[] | string[][],
    onLoad?: (result: T, ...args: any[]) => void,
    onProgress?: (event: ProgressEvent) => void,
    onError?: (event: ErrorEvent) => void,
  ): unknown
  loadAsync(url: string | string[] | string[][], onProgress?: (event: ProgressEvent) => void): Promise<T>
}

export function useLoader<
  T,
  U extends string | string[] | string[][],
  L extends LoaderProto<T>,
  R = LoaderReturnType<T, L>,
>
...
useLoader.preload = function <T, U extends string | string[] | string[][], L extends LoaderProto<T>>
...
useLoader.clear = function <T, U extends string | string[] | string[][], L extends LoaderProto<T>>

It's not pretty but it works.

@CodyJasonBennett CodyJasonBennett changed the title [v9] fix(types): loosen Loader type [v9] fix(useLoader): properly infer Loader types Feb 27, 2023
@krispya
Copy link
Member

krispya commented Mar 15, 2023

How is this looking? Last I tested it worked as intended.

@krispya krispya merged commit dde49a5 into v9 Jun 22, 2023
@CodyJasonBennett CodyJasonBennett deleted the fix/loosen-loader-type branch June 22, 2023 02:53
@IWSR
Copy link

IWSR commented Nov 16, 2023

So this would be url: string | string[] | string[][]? That's a bit unfortunate, but good if TS picks it up.

It ends up looking this, including for useLoader:

export interface Loader<T> extends THREE.Loader {
  load(
    url: string | string[] | string[][],
    onLoad?: (result: T, ...args: any[]) => void,
    onProgress?: (event: ProgressEvent) => void,
    onError?: (event: ErrorEvent) => void,
  ): unknown
  loadAsync(url: string | string[] | string[][], onProgress?: (event: ProgressEvent) => void): Promise<T>
}

export function useLoader<
  T,
  U extends string | string[] | string[][],
  L extends LoaderProto<T>,
  R = LoaderReturnType<T, L>,
>
...
useLoader.preload = function <T, U extends string | string[] | string[][], L extends LoaderProto<T>>
...
useLoader.clear = function <T, U extends string | string[] | string[][], L extends LoaderProto<T>>

It's not pretty but it works.

Hello, I'm having difficulty understanding the role of the third and fourth parameters in the type declaration for useLoader. Do users need to provide these parameters? Is it mandatory for users to specify them? And could you provide an example?

@CodyJasonBennett
Copy link
Member Author

They are not required to be specified in user-land, and I wish there was a way to hide them. They are populated implicitly through inference.

@IWSR
Copy link

IWSR commented Nov 16, 2023

They are not required to be specified in user-land, and I wish there was a way to hide them. They are populated implicitly through inference.

Thank you for your response, but I'm currently facing the issue shown in the image below. I'm confused about how to address it because the third and fourth items are mandatory in the type declaration.

image

The error message is 'Expected 3-4 type arguments, but got 2.ts(2558)' . And I reviewed the documentation again, and it turns out the last two items are not mandatory either.

@CodyJasonBennett
Copy link
Member Author

Is this since TypeScript 5? Sounds like a bug, will have to fix for latest versions.

@IWSR
Copy link

IWSR commented Nov 19, 2023

Is this since TypeScript 5? Sounds like a bug, will have to fix for latest versions.

My ts version is "^5.0.2". Do I need to open a separate issue for 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 this pull request may close these issues.

3 participants