Support for multiple method signatures#2227
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.
Any idea why this got moved up? @saschanaz
There was a problem hiding this comment.
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.
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.
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.
BTW you can also do
| param extensionName type="\"ANGLE_instanced_arrays\"" | |
| param extensionName type=#""ANGLE_instanced_arrays""# |
saschanaz
left a comment
There was a problem hiding this comment.
Going to the right direction
saschanaz
left a comment
There was a problem hiding this comment.
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.
Meh, one is enough, we don't have to cover every overload.
|
|
||
| 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.
This won't work if ext_ is any
There was a problem hiding this comment.
Can you clarify?
It is never any
There was a problem hiding this comment.
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.
Try giving "ANGLE_illegal" instead for getExtension.
There was a problem hiding this comment.
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... |
|
Anyway I guess this PR is fine, but needs a main merge |

#2053