HTMLCanvasElement.getContext() missing "webgl" type in lib.d.ts

[GoToLoop]

getContext(contextId: "2d"): CanvasRenderingContext2D;
getContext(contextId: "experimental-webgl"): WebGLRenderingContext;
getContext(contextId: string, ...args: any[]): CanvasRenderingContext2D | WebGLRenderingContext;

Overloaded getContext(contextId: "experimental-webgl"): WebGLRenderingContext; is missing "webgl" according to these reference sites below:
https://developer.Mozilla.org/en-US/docs/Web/API/HTMLCanvasElement/getContext
http://www.JavaScripture.com/HTMLCanvasElement

Also all the overloaded methods got an optional contextAttributes?: {} parameter.
And I don't think ...args: any[] represents the 2nd parameter correctly.
This is my change proposal for them btW: :-D

getContext(contextType: "2d", contextAttributes?: {}): CanvasRenderingContext2D;
getContext(contextType: "webgl" | "experimental-webgl", contextAttributes?: {}): WebGLRenderingContext;
getContext(contextType: string, contextAttributes?: {}): CanvasRenderingContext2D | WebGLRenderingContext;

[mhegazy]

PRs are welcomed. here is some details on adding these type definitions: https://github.com/Microsoft/TypeScript/blob/master/CONTRIBUTING.md#contributing-libdts-fixes

[rohitverma007]

Hi, I am fairly new to TypeScript and OSS, and this looks like a great way for me to start!
I would like to work on this, if nobody has already done so.
Although, I do have some more recommendations.
The contextAttributes can have various optional attributes according to the context ("2d" vs "webgl") as stated in the docs. There is also a new "webgl2" context that is not yet ready for production but, I think it could be included as well. With that in mind, here is what I was thinking of updating it to:

getContext(contextType: "2d", contextAttributes?: {alpha?: boolean, willReadFrequently?: boolean, storage?: string}): CanvasRenderingContext2D;
getContext(contextType: "webgl" | "experimental-webgl" | "webgl2" | "experimental-webgl2" , contextAttributes?: {alpha?: boolean, depth?: boolean, stencil?: boolean, antialias?: boolean, premultipliedAlpha?: boolean, preserveDrawingBuffer?: boolean, failIfMajorPerformanceCaveat?: boolean}): WebGLRenderingContext | WebGL2RenderingContext;
getContext(contextType: string, contextAttributes?: {}): CanvasRenderingContext2D | WebGLRenderingContext | WebGL2RenderingContext;

Let me know how you guys feel about that.

[mhegazy]

I would split into two signatures, one for "webgl" | "experimental-webgl" and another for "webgl2" | "experimental-webgl2" since they return two different types. this way the user does not have to check/cast the result.

[rohitverma007]

right, makes sense, I'll get started on this then :)

[GoToLoop]

My initial proposal was trying to be simple while still describing the overload signatures correctly.
But since it's been decided to go for some full blown complete specification, here's my re-proposal now:

getContext(contextType: "2d", contextAttributes?: {
  alpha?: boolean; willReadFrequently?: boolean; storage?: string;
  [attrib: string]: boolean | string; }): CanvasRenderingContext2D;

getContext(contextType: "webgl" | "experimental-webgl", contextAttributes?: {
  alpha?: boolean; depth?: boolean; stencil?: boolean; antialias?: boolean;
  premultipliedAlpha?: boolean; preserveDrawingBuffer?: boolean; failIfMajorPerformanceCaveat?: boolean;
  [attrib: string]: boolean; }): WebGLRenderingContext;

getContext(contextType: "webgl2" | "experimental-webgl2", contextAttributes?: {
  alpha?: boolean; depth?: boolean; stencil?: boolean; antialias?: boolean;
  premultipliedAlpha?: boolean; preserveDrawingBuffer?: boolean; failIfMajorPerformanceCaveat?: boolean;
  [attrib: string]: boolean; }): WebGL2RenderingContext;

getContext(contextType: string, contextAttributes?: { [prop: string]: boolean | string; }):
  CanvasRenderingContext2D | WebGLRenderingContext | WebGL2RenderingContext;

And maybe in order to shorten the WebGL overloads, how about creating some interface for parameter contextAttributes?:

interface WebGLAttributesObj {
  alpha?: boolean;
  depth?: boolean;
  stencil?: boolean;
  antialias?: boolean;
  premultipliedAlpha?: boolean;
  preserveDrawingBuffer?: boolean;
  failIfMajorPerformanceCaveat?: boolean;

  [attrib: string]: boolean;
}

getContext(contextType: "webgl"  | "experimental-webgl",  contextAttributes?: WebGLAttributesObj): WebGLRenderingContext;
getContext(contextType: "webgl2" | "experimental-webgl2", contextAttributes?: WebGLAttributesObj): WebGL2RenderingContext;

P.S.: In order to have WebGL2RenderingContext type, we're gonna need some stub for it as well:

interface WebGL2RenderingContext extends WebGLRenderingContext {}

[rohitverma007]

The re-proposal looks good.
But, I am not too sure about creating an interface for the contextAttributes as wouldn't it be possible that the webgl2 contextattributes could have more options than webgl at some point and hence that interface could no longer be used?

[GoToLoop]

If or when that happens, it's as simple as making another interface extending that.
Anyways, WebGL2RenderingContext is more like a draft right now and it's Firefox exclusive so far.

[rohitverma007]

@GoToLoop sounds good, I guess I will ignore the webgl2 stuff for now then, but still use an interface for the webgl context,
@mhegazy kind of stuck at something, any help would be appreciated :).
I noticed there is already a WebGLContextAttributes ( threw me off a bit since I named it the exact same before realizing it already existed :)) , to add a new property to the existing WebGLContextAttributes, I am adding the following to addedTypes.json:

    {
        "kind": "property",
        "interface": "WebGLContextAttributes",
        "name": "failIfMajorPerformanceCaveat?",
        "type": "boolean"
    },

But the dom.generated.d.ts is not updating the existing interface... does it need to go into overridingTypes.json instead? I have tried that as well.. doesn't work either.
The overriding of the getContext methods works fine, I simply modified the already existing override in overridingTypes.json.

[mhegazy]

But the dom.generated.d.ts is not updating the existing interface... does it need to go into overridingTypes.json instead? I have tried that as well.. doesn't work either.
The overriding of the getContext methods works fine, I simply modified the already existing override in overridingTypes.json.

Sorry for the delay. @zhengbli can you help @rohitverma007 out.

[zhengbli]

@rohitverma007 You did the right thing to add the content in addedTypes.json file. Though the script has a bug that doesn't pick up the change for dictionaries in the spec, I'm fixing it and will update you soon.

[zhengbli]

@rohitverma007 the script has been updated. Can you try with the new content in addedTypes.json now? Thank you!