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

feat (Nodes): more tslFn related typings #744

Merged
merged 10 commits into from
Apr 12, 2024
Merged

Conversation

0b5vr
Copy link
Contributor

@0b5vr 0b5vr commented Jan 16, 2024

I used The TSL editor to confirm whether the type matches the actual behavior.

- Fix type of mx_hsv, mx_noise, MaterialXNodes
- Add missing methods of mx_noise
- Add mx_transform_color
- Update test cases
    - the directory name was `materials` instead of `materialx`, fixed this

`mx_noise.js` exports requires all arguments to exist, while `MaterialXNodes.js` exports don't
@0b5vr 0b5vr self-assigned this Jan 16, 2024
I forgot to put a thing in `assetSwizzable()`
@0b5vr
Copy link
Contributor Author

0b5vr commented Jan 17, 2024

The test says:

1> /home/runner/work/three-ts-types/three-ts-types/DefinitelyTyped/types/three/test/unit/examples/jsm/nodes/ShaderNode/ShaderNode.ts

1>   75:23  error  TypeScript@4.6, 4.7, 4.8, 4.9, 5.0 compile error: 
1> Type of property 'cache' circularly references itself in mapped type '{ [Key in keyof NodeElements]: NodeElements[Key] extends (node: CacheNode, ...args: infer Args) => infer R ? (...args: Args) => R : never; }'      @definitelytyped/expect
1>   75:23  error  TypeScript@4.6, 4.7, 4.8, 4.9, 5.0 compile error: 
1> Type of property 'context' circularly references itself in mapped type '{ [Key in keyof NodeElements]: NodeElements[Key] extends (node: ContextNode, ...args: infer Args) => infer R ? (...args: Args) => R : never; }'  @definitelytyped/expect

I have no idea what to do...

@Methuselah96
Copy link
Contributor

That is super weird. Maybe just a bug in TypeScript <=5.0? Does the error go away if you just remove the lines it's producing an error on?

@0b5vr
Copy link
Contributor Author

0b5vr commented Jan 23, 2024

That is super weird. Maybe just a bug in TypeScript <=5.0? Does the error go away if you just remove the lines it's producing an error on?

I'm sorry for the late reply. Yes, the error disappears once I comment out the lines. However, the issue persists on the end developer's code side 😖

@0b5vr
Copy link
Contributor Author

0b5vr commented Mar 12, 2024

The Type of property 'cache' circularly references itself in mapped type error disappears once I bump the TypeScript version to 5.1.3 or above.
We might want to introduce some workarounds for 5.0 or below?

@Methuselah96
Copy link
Contributor

Yeah, it's rather unfortunate. I'd be interested to know what kind of workarounds would make it work for 5.0 and below, and how it would impact the types for the user.

@0b5vr
Copy link
Contributor Author

0b5vr commented Apr 2, 2024

I'd be interested to know what kind of workarounds would make it work for 5.0 and below, and how it would impact the types for the user.

How do you think if we go ask for a help in DefinitelyTyped issues...? low confidence.
They are probably experts of such type troubles.

@Methuselah96
Copy link
Contributor

In my experience, those often don't get responded to. You might get a better response using the TypeScript Community Discord server (either the #definitely-typed channel or another channel). Or you could try asking about it on StackOverflow.

My schedule frees up in the next couple weeks so I should have some time to look into this soon regardless.

@0b5vr
Copy link
Contributor Author

0b5vr commented Apr 2, 2024

ok thanks for the info! I might try either tomorrow

0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Apr 4, 2024
The error is since `ContextNode` already has a member `.context` and `ShaderNodeObject` also gives it a method `.context`

See: three-types#744 (comment)
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Apr 4, 2024
The error is since `ContextNode` already has a member `.context` and `ShaderNodeObject` also gives it a method `.context`

See: three-types#744 (comment)
0b5vr added a commit to 0b5vr/three-ts-types that referenced this pull request Apr 4, 2024
The error is since `ContextNode` already has a member `.context` and `ShaderNodeObject` also gives it a method `.context`

See: three-types#744 (comment)
The error is since `ContextNode` already has a member `.context` and `ShaderNodeObject` also gives it a method `.context`

See: three-types#744 (comment)

I believe the behavior that the node class properties takes precedance than NodeElements is same as the actual implementation

See: https://github.com/mrdoob/three.js/blob/r163/examples/jsm/nodes/shadernode/ShaderNode.js
Copy link
Contributor

@Methuselah96 Methuselah96 left a comment

Choose a reason for hiding this comment

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

Nice work hunting that down!

@Methuselah96 Methuselah96 merged commit 4b13e2a into three-types:master Apr 12, 2024
3 checks passed
@0b5vr 0b5vr deleted the tslfn2 branch April 15, 2024 07:45
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.

None yet

2 participants