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 6 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
Author

Choose a reason for hiding this comment

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

🙇 Thanks, fixed

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 83.12%. Comparing base (8df5911) to head (0d0b4dd).

Files with missing lines 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.21%   83.12%   -0.09%     
==========================================
  Files         120      120              
  Lines        8066     8077      +11     
  Branches     1937     1937              
==========================================
+ Hits         6712     6714       +2     
- Misses       1354     1363       +9     

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

@barlock
Copy link
Author

barlock commented Aug 9, 2024

@mister-ben updated! Sorry I missed the comments

@barlock
Copy link
Author

barlock commented Aug 9, 2024

I don't have access to see why netlify is failing, can you post logs?

@gkatsev
Copy link
Member

gkatsev commented Aug 9, 2024

not sure netlify failed, I retried it and it worked: https://deploy-preview-8545--videojs-docs.netlify.app/
Not sure why it isn't reporting status back on this pr either, but that won't be a blocker on merging assuming everything else is good.

@barlock
Copy link
Author

barlock commented Sep 11, 2024

Anything missing to get this merged?

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.

4 participants