Skip to content

fix: improve types and allow augmentation#8545

Open
barlock wants to merge 7 commits into
videojs:mainfrom
barlock:type-updates
Open

fix: improve types and allow augmentation#8545
barlock wants to merge 7 commits into
videojs:mainfrom
barlock:type-updates

Conversation

@barlock

@barlock barlock commented Jan 4, 2024

Copy link
Copy Markdown

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

@welcome

welcome Bot commented Jan 4, 2024

Copy link
Copy Markdown

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

@uzbeki uzbeki left a comment

Copy link
Copy Markdown

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
Comment thread src/js/component.js Outdated
* The `Component` class to register.
*
* @return {Component}
* @return {C}

Copy link
Copy Markdown
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
Copy Markdown
Author

Choose a reason for hiding this comment

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

🙇 Thanks, fixed

@codecov

codecov Bot commented Jul 9, 2024

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 18.18182% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.16%. Comparing base (d2b9d5c) to head (651dc44).
⚠️ Report is 56 commits behind head on main.

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.85%   83.16%   -0.70%     
==========================================
  Files         120      120              
  Lines        8097     8108      +11     
  Branches     1944     1944              
==========================================
- Hits         6790     6743      -47     
- Misses       1307     1365      +58     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@barlock

barlock commented Aug 9, 2024

Copy link
Copy Markdown
Author

@mister-ben updated! Sorry I missed the comments

@barlock

barlock commented Aug 9, 2024

Copy link
Copy Markdown
Author

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

@gkatsev

gkatsev commented Aug 9, 2024

Copy link
Copy Markdown
Member

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

barlock commented Sep 11, 2024

Copy link
Copy Markdown
Author

Anything missing to get this merged?

@deguif

deguif commented Jan 16, 2025

Copy link
Copy Markdown

Do you think this can be merged soon?

@PierreTraversFamileoPro

PierreTraversFamileoPro commented Jan 16, 2025

Copy link
Copy Markdown

Do you think this can be merged soon?

👍 : 1

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.

6 participants