webvr-api: Make webvr-api consistent with latest lib.es6.d.ts#24690
webvr-api: Make webvr-api consistent with latest lib.es6.d.ts#24690sandersn merged 3 commits intoDefinitelyTyped:masterfrom
Conversation
|
@efokschaner Thank you for submitting this PR! 🔔 @lostfictions - please review this PR in the next few days. Be sure to explicitly select If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead. |
|
don't really have time/availability/capacity to review most DT PRs these days, unfortunately :( is there a way i can explicitly opt out of being tagged for review? it's been happening a few times a week for me for the last month or so and i'd love to help but i don't get paid for this and am already struggling to get by :( @paulvanbrenk @sandersn @Andy-MS (sorry if you're not DT maintainers, there doesn't seem to be a maintainer list in the readme) |
|
@lostfictions You can remove yourself from author lists, which is, unfortunately, another set of PRs to submit. In this case, @efokschaner can you remove @lostfictions from the author list and add yourself? |
|
I've gone ahead and changed the author. |
|
After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience! |
| new(): VRFrameData; | ||
| }; | ||
|
|
||
| interface VRPose { |
There was a problem hiding this comment.
why not make VRDisplayCapabalities, VREyeParameters, VRFieldOfView, VRPose and VRFrameData declare class instead? You could just change the interface lines and get rid of the new declare vars entirely.
There was a problem hiding this comment.
I think i've done it precisely the same as they are in https://github.com/Microsoft/TypeScript/blob/master/lib/lib.es6.d.ts
So although I agree it's a weird pattern I see no reason not to match the official defs to avoid confusion.
Does that seem sound?
|
|
||
| // Typescript doesn't allow redefinition of type aliases even if they match, | ||
| // thus the _dt_alias to signal this being an alias for the use of DefinitelyTyped | ||
| type VRDisplayEventReason_dt_alias = "mounted" | "navigation" | "requested" | "unmounted"; |
There was a problem hiding this comment.
where are these defined? Can you just change the originals?
There was a problem hiding this comment.
oh nm this is in lib.dom.d.ts. I had no idea this stuff was built-in to the browser.
There was a problem hiding this comment.
They're defined in https://github.com/Microsoft/TypeScript/blob/master/lib/lib.es6.d.ts
We wouldn't want to change those. Think of this typing as a backport of the official definitions for TS versions that don't yet have them. And these typings in here need to work even when the official definitions are visible and doing it this way works. The type alias' don't conflict nor do the functions they're used by.
|
@efokschaner One or more reviewers has requested changes. Please address their comments. I'll be back once they sign off or you've pushed new commits. Thank you! |
The old definition for
addEventListener(type: "vrdisplaypresentchange"had tsc errors because it did not match the latest lib.es6.d.ts 'sthisdeclaration.This alerted me to some other inconsistencies which the spec and with the builtin declarations that I've also fixed.
npm test.)npm run lint package-name(ortscif notslint.jsonis present).If changing an existing definition:
This change should be consistent with both:
tslint.jsoncontaining{ "extends": "dtslint/dt.json" }.