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

bugfix: Fix typings #2242

Closed
wants to merge 1 commit into from

Conversation

ccreed1988
Copy link

Related issue: #2241

I wanted to fix some of the typing issues I saw when I was trying to build my Angular project. The issues are related to this library (luma.gl) and deck.gl

@ibgreen ibgreen requested a review from felixpalmer September 3, 2024 17:34
@ibgreen
Copy link
Collaborator

ibgreen commented Sep 3, 2024

@felixpalmer I am not sure we want to use any if we can avoid it. Is there a better way? unknown?

@ccreed1988
Copy link
Author

I think unknown should work. I can update the PR if that is the only thing you saw.

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 3, 2024

@ccreed1988 Would like a comment from @felixpalmer who has been working on the typs.

Also the build is failing with type errors, we would need to fix those before merging.

@@ -24,8 +24,7 @@ export class ShaderInputs<
* The map of modules
* @todo should should this include the resolved dependencies?
*/
// @ts-expect-error Fix typings
modules: Readonly<{[P in keyof ShaderPropsT]: ShaderModule<ShaderPropsT[P]>}>;
modules: Readonly<{[P in keyof ShaderPropsT]: ShaderModule<Record<string, any>>}>;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making this change throws away the knowledge of the type the props of a ShaderModule has, which seems like a shame.

Perhaps we are trying to be too clever with the types here though and for now we can go with ShaderModule<Record<string, unknown>>, what do you think @ibgreen? Realistically, I'm not going to have the bandwidth to look at this for a few weeks

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree that having types for the shader module props is a major improvement in v9 that we don't want to give up without a deep understanding of what is going wrong.

What we could perhaps consider is landing this on the 9.0-release branch instead of master. But still I would much rather we root cause and fix the issue rather than just remove typing.

@ibgreen
Copy link
Collaborator

ibgreen commented Sep 20, 2024

Closing as we don't want to remove typings, open to other ideas to fix this.

@ibgreen ibgreen closed this Sep 20, 2024
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