-
Notifications
You must be signed in to change notification settings - Fork 455
Support for multiple method signatures #2227
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
base: main
Are you sure you want to change the base?
Conversation
|
Thanks for the PR! This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged. |
| /** [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebGLRenderingContext/getError) */ | ||
| getError(): GLenum; | ||
| /** [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebGLRenderingContext/getExtension) */ | ||
| getExtension(name: string): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any idea why this got moved up? @saschanaz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this PR is now merging signature while originally this was using additionalSignatures, which has ordering difference. But the different order doesn't seem to affect TS behavior, while I thought it would matter, or at least it did at some point (microsoft/TypeScript#1860 (comment)). Maybe that changed.
@jakebailey, any idea?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could flip things around in the emitter.
| /** [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebGLRenderingContext/getError) */ | ||
| getError(): GLenum; | ||
| /** [MDN Reference](https://developer.mozilla.org/docs/Web/API/WebGLRenderingContext/getExtension) */ | ||
| getExtension(name: string): any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this PR is now merging signature while originally this was using additionalSignatures, which has ordering difference. But the different order doesn't seem to affect TS behavior, while I thought it would matter, or at least it did at some point (microsoft/TypeScript#1860 (comment)). Maybe that changed.
@jakebailey, any idea?
inputfiles/patches/webgl.kdl
Outdated
| method getExtension { | ||
| signatures { | ||
| _ { | ||
| param extensionName type="\"ANGLE_instanced_arrays\"" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW you can also do
| param extensionName type="\"ANGLE_instanced_arrays\"" | |
| param extensionName type=#""ANGLE_instanced_arrays""# |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which one would you prefer @saschanaz?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Going to the right direction
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, assuming @jakebailey is okay with the overload ordering.
|
I have fixed the ordering issue @saschanaz |
|
I don't like the last change and canceling my approval. Let's ask Jake first? |
|
Sure @saschanaz |
|
That will bring my approval back 👍 |
|
I have reverted it @saschanaz |
|
Hey @saschanaz |
This. |
unittests/files/webgl.ts
Outdated
| // TypeScript should infer: ANGLE_instanced_arrays | null | ||
| assertType<ANGLE_instanced_arrays | null>(ext_ANGLE_instanced_arrays); | ||
|
|
||
| const ext_EXT_blend_minmax = gl.getExtension("EXT_blend_minmax"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Meh, one is enough, we don't have to cover every overload.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
|
|
||
| const ext_ANGLE_instanced_arrays = gl.getExtension("ANGLE_instanced_arrays"); | ||
| // TypeScript should infer: ANGLE_instanced_arrays | null | ||
| assertType<ANGLE_instanced_arrays | null>(ext_ANGLE_instanced_arrays); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work if ext_ is any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you clarify?
It is never any
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The intention of the test is to check it's not any. But if it becomes any this still won't fail.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try giving "ANGLE_illegal" instead for getExtension.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this isn't a correct way to test this, unfortunately. If this is any, it's legal to cast to anything.
Removed multiple unused WebGL extension assertions.
|
I have added more types @saschanaz |
|
Cool, thanks! |
|
Isn't it critical that the overloads are ordered most specific to least specific? |
I think that changed since I wrote tests, and they worked |
|
I would be very surprised if so, given we are supposed to walk top to bottom. How much work is it to avoid this? Is it challenging? (I am not asking for it to be changed yet) |
|
I tried to avoid it but @saschanaz didn't like the proposed solution Another solution is to just use overrideSignature like the original |
I thought so but it works nonetheless. I thought you would know why... |
#2053