fix(types): Adds track and track list types to typescript exports #8486
Conversation
|
💖 Thanks for opening this pull request! 💖 Things that will help get your PR across the finish line:
We get a lot of pull requests on this repo, so please be patient and we will get back to you as soon as we can. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8486 +/- ##
==========================================
- Coverage 82.71% 82.62% -0.09%
==========================================
Files 113 113
Lines 7589 7608 +19
Branches 1826 1836 +10
==========================================
+ Hits 6277 6286 +9
- Misses 1312 1322 +10 ☔ View full report in Codecov by Sentry. |
|
Thank you, this is very helpful. And thank you for considering the api docs output, the overarching problem is that Typescript doesn't understand all jsdoc, and has innovated some jsdoc syntac that jsdoc itself doesn't support. The code coverage drop is unavoidable. |
No worries. Sorry for the delay, I think I have some ideas to pursue on code coverage drop. I'll get on that :). |
| @@ -5163,51 +5163,56 @@ class Player extends Component { | |||
| * | |||
| * @link https://html.spec.whatwg.org/multipage/embedded-content.html#videotracklist | |||
| * | |||
| * @return {VideoTrackList} | |||
| * @returns {import("./tracks/video-track-list").VideoTrackList} | |||
There was a problem hiding this comment.
We can use the better import syntax now
/** @import { VideoTrackList } from './tracks/video-track-list' */
...
@returns {VideoTrackList}
Description
We love Video.js where I work and are currently using TypeScript. There are several areas where the TypeScript support for Video.js is lacking, even to the point of some of the guides on the website not being able to be compiled by TypeScript).
Currently, the guides around Tracks and Track list (like Audio Tracks for example: https://videojs.com/guides/audio-tracks/#listen-for-an-audio-track-becoming-enabled) can't be used out of the box with video.js and TypeScript. This PR doesn't fully address the issue of not being able to use the code verbatim, but it exposes the main types that will better unblock TypeScript developers on using these scenarios.
I kept the changes small to try to keep any conversation focused before moving on to other missing issues. This has come up in other issues: #8466.
Many of the issues I see are around either: types that weren't exported from the package (and hence, can't be referenced/used from TypeScript code), or from JSDoc comments that aren't attached to any specific variable or type (and hence, it's picked up by the TypeScript compiler when generating outgoing types).
Usable (and shown in code completion) by TypeScript in VSCode.
JSDocs on website still show correct types (some changes that TypeScript wanted resulted in a changing of the types displayed in the JSDoc website, so I explicitly did NOT do any of those changes in this PR without any further discussion).
The change exposes the types just like Player and others that were already exposed in video.js. This allows the specific list types to be referenced and used like this in TypeScript:
This would have failed before as the Player type didn't export
audioTracks/textTracks/videoTracks, and video.js also didn't expose the matching*Listtypes either which made proper isolated testing in TypeScript much more difficult without jumping through hoops and creating our own types definitions.Hopefully these will be fairly non-controversial.
Specific Changes proposed
This first proposed change is based on a quick fix mentioned in the thread above: #8466.
The change resolves around these specific elements:
Add placeholder prototypes before the
videoTracks/audioTracks/textTracksare added to the prototypeTypeScript compiler doesn't appear to pull JSDoc comments unless they are attached to something in the abstract syntax tree instead of floating in space.
Export specific track types from their files and reference those types via
importof JSDocThis is so TypeScript pulls them in when generating types. Leaving it just
AudioTrackListlike before resulted in the TypeScript compiler just generating theaudioTracks/videoTracks/etc. properties as returninganyfrom their methods.There's a different way this can be done by importing the types, but it appears to then export them as well from the player.js file. That felt like a side-effect that wouldn't be appreciated, so I tried this less invasive way to keep the JavaScript interface as identical as possible from before.
I also ensured the above change doesn't break existing JSDoc documentation published to the website by generating the docs and exploring them locally.
Export those newly exported types via src/js/video.js so they are exported and usable from TypeScript
Other Notes
There are some other changes needed around the TrackList to let it work out of the box from TypeScript (as there are issues around it not being documented as ArrayLike for looping over returned tracks). Those changes were starting to be a bit more invasive though, so I saved those for later and focused on just exposing these core types.
Obviously, feedback is most welcome :).
Requirements Checklist