Add TypeScript declarations to app-media. #48
Conversation
| directories: | ||
| - node_modules | ||
| env: | ||
| global: [] |
There was a problem hiding this comment.
I did not have the sauce-credentials.json file, so I was not able to generate these keys
| * [here](https://w3c.github.io/mediacapture-image/##photosettings-section). | ||
| */ | ||
| readonly photoSettings: PhotoSettings; | ||
| fillLightMode: Polymer.AppMedia.FillLightMode|null; |
There was a problem hiding this comment.
Does a type need to be generated for Polymer.AppMedia.FillLightMode?
There was a problem hiding this comment.
@aomarks I added the @namespace for Polymer.AppMedia, but it does not seem to detect that the other modes are also added to this namespace. Do you maybe know what is going wrong here?
| readonly trackConstraints: MediaTrackConstraints; | ||
| whiteBalanceMode: string|null|undefined; | ||
| colorTemperature: number|null|undefined; | ||
| exposureMode: Polymer.AppMedia.MeteringMode|null; |
There was a problem hiding this comment.
Does a type need to be generated for Polymer.AppMedia.MeteringMode?
app-media-image-capture.d.ts
Outdated
| * | ||
| * @returns Promise<Blob> | ||
| */ | ||
| takePhoto(): any; |
There was a problem hiding this comment.
I think both forms are accepted by closure compiler
There was a problem hiding this comment.
Yeah, both are valid. I used @returns in the generator (when emitting documentation for the return type if there is any) because it seemed to be the canonical form for JSDoc (http://usejsdoc.org/tags-returns.html), although for Closure the canonical form is @return (https://github.com/google/closure-compiler/wiki/Annotating-JavaScript-for-the-Closure-Compiler#return-type-description), and that's what we use most of the time in the elements, so I now I'd prefer @return. But it didn't seem worth the churn of rewriting all these files over that.
There was a problem hiding this comment.
The actual problem here and below is that the source is missing curly braces around the type: @return {Promise<Blob>} (@returns would also work).
There was a problem hiding this comment.
Oh and... that would be @return {!Promise<!Blob>} because object types in Closure are nullable by default.
There was a problem hiding this comment.
Thanks for the clarification @aomarks 🍻
@TimvdLippe would you be willing to fix the source annotation as part of this change?
There was a problem hiding this comment.
Yes I will push an update tomorrow
app-media-image-capture.d.ts
Outdated
| * | ||
| * @returns Promise<ImageBitmap> | ||
| */ | ||
| grabFrame(): any; |
| /** | ||
| * An ImageCapture instance associated with the selected video track. | ||
| */ | ||
| readonly imageCapture: ImageCapture|null; |
There was a problem hiding this comment.
I don't think the ImageCapture type is defined in the TypeScript standard libs, so this shouldn't compile. We could define it by manually writing an extra-types.d.ts and including it with addReferences in gen-tsd.json, or ignore it by putting "renameTypes": {"ImageCapture": "any"} in the config file.
There was a problem hiding this comment.
I contributed the closure types for this constructor and its related settings object to the closure externs. Would it be possible to make use of generated types based on that somehow?
Alternatively, would it be possible to make a similar contribution to the TypeScript standard library?
There was a problem hiding this comment.
Yes I think we should upstream this into the lib.dom.d.ts typings
There was a problem hiding this comment.
That would be great. I wouldn't be opposed to doing the any rename for now since getting those into TS could take a while.
There was a problem hiding this comment.
It would be nice to see a first step as part of this change; maybe file an issue?
|
Per https://github.com/Microsoft/TSJS-lib-generator#when-should-a-dom-api-be-included-here DefintelyTyped was the location, where I found https://github.com/DefinitelyTyped/DefinitelyTyped/blob/1882fc533c95e710336c4f6236ea8caf8c6a28a4/types/w3c-image-capture/index.d.ts However, we can't just include these types, as we first have to address https://github.com/Polymer/gen-typescript-declarations/issues/104 |
|
I am closing this PR. We can autogenerate a new one later once the above issues are resolved. |
This PR adds TypeScript declarations generated by https://github.com/Polymer/gen-typescript-declarations/
These declarations can be re-generated by running
npm run update-types.Tracker: https://github.com/Polymer/gen-typescript-declarations/issues/79