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: improve types and allow augmentation #8545

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

barlock
Copy link

@barlock barlock commented Jan 4, 2024

Description

Fix a few types in player and plugin and allow typescript module augmentation so plugins can modify the players type as expected.

Specific Changes proposed

For type fixes, I used similar patterns found elsewhere in the codebase, just applied following the same pattern. For module augmentation, this pattern should be used:

declare module 'video.js/dist/types/player' {
    // videojs-contrib-quality-levels
    qualityLevels: {
      VERSION: string;
      (): QualityLevelList;
    };
  }
}

Ideally one wouldn't need to reach into dist but I'm unable to do that without created a named export of Player on video.js which I didn't think was wanted. Happy to make the change though. JSDoc seems to prevent me from only exporting the type of Player without exporting the whole class as well.

Requirements Checklist

Copy link

welcome bot commented Jan 4, 2024

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

Copy link

@uzbeki uzbeki left a comment

Choose a reason for hiding this comment

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

that looks like a decent addition to its types.

@mister-ben mister-ben added the ts TypeScript label Apr 10, 2024
* The `Component` class to register.
*
* @return {Component}
* @return {C}
Copy link
Contributor

Choose a reason for hiding this comment

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

The jsdoc output is bad here, has a {C} type that's not defined
image

whereas the getComponent you've modified below in video.js is displayed correctly, with a {Class.<Component>}
image

Copy link

codecov bot commented Jul 9, 2024

Codecov Report

Attention: Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.

Project coverage is 82.98%. Comparing base (970faa8) to head (25e937d).

Files Patch % Lines
src/js/player.js 0.00% 5 Missing ⚠️
src/js/plugin.js 33.33% 4 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8545      +/-   ##
==========================================
- Coverage   83.07%   82.98%   -0.09%     
==========================================
  Files         120      120              
  Lines        8022     8033      +11     
  Branches     1925     1925              
==========================================
+ Hits         6664     6666       +2     
- Misses       1358     1367       +9     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

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.

None yet

3 participants