Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(types): Adds track and track list types to typescript exports #8486

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jolson88
Copy link

@jolson88 jolson88 commented Nov 8, 2023

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).

Screen Shot 2023-11-01 at 2 44 59 PM

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:

const trackList: videojs.VideoTrackList = myPlayer.videoTracks();

This would have failed before as the Player type didn't export audioTracks/textTracks/videoTracks, and video.js also didn't expose the matching *List types 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/textTracks are added to the prototype

TypeScript 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 import of JSDoc
This is so TypeScript pulls them in when generating types. Leaving it just AudioTrackList like before resulted in the TypeScript compiler just generating the audioTracks/videoTracks/etc. properties as returning any from 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

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chrome, Firefox, IE)
    • Unit Tests updated or fixed
    • Docs/guides updated
    • Example created (starter template on JSBin)
  • Reviewed by Two Core Contributors

Copy link

welcome bot commented Nov 8, 2023

💖 Thanks for opening this pull request! 💖

Things that will help get your PR across the finish line:

  • Run npm run lint -- --errors locally to catch formatting errors earlier.
  • Include tests when adding/changing behavior.
  • Include screenshots and animated GIFs whenever possible.

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.

@jolson88 jolson88 mentioned this pull request Nov 8, 2023
7 tasks
Copy link

codecov bot commented Nov 8, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.62%. Comparing base (d535e16) to head (95e71f4).
Report is 82 commits behind head on main.

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.
📢 Have feedback on the report? Share it here.

@mister-ben
Copy link
Contributor

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.

@jolson88
Copy link
Author

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 :).

@mister-ben mister-ben added browser bug ts TypeScript and removed browser bug labels May 29, 2024
@@ -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}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can use the better import syntax now

/** @import { VideoTrackList } from './tracks/video-track-list' */

...

@returns {VideoTrackList}

@nlfurniss
Copy link

@jolson88 if you give me edit permission for this PR i can make the requested changes, otherwise I can open a new PR based on this one

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ts TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants