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

refactor(types): Type enhancements #8968

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

Conversation

mister-ben
Copy link
Contributor

@mister-ben mister-ben commented Jan 23, 2025

Several changes for Typescript.

  • Adds a jsdoc plugin to not break on @template {xyz} T. Replaces uses of T with Class.<xyz>
  • Adds definitions for ComponentOptions, SourceObject, PlayerOptions, MediaObject
  • Removes further uses of ~ namespacing in jsdoc, which tsc fails to understand
  • Changes track kinds to simple union types. Change the existing definitions to enums so we can use them as object lookups without conflicting with the standard definitions.
  • Use native TextTrackCue, TextTrackCue definitions.
  • Exports Player, PlayerOptions and Plugin from video.js.
    • This makes it easier to add to the object than at present e.g. in a plugin to declare module 'video.js' { interface Player {
    • This sets a precedent - what else could/should be exported at this level? Should the names be more verbose?
  • Adds additional jsdoc on for track and readyState in HTMLTrackElement, as tsc doesn't understand documentation for properties added with Object.defineProperties . Fixes HTMLTrackElement class provided missing two properties as stated in MDN #8959

Copy link

codecov bot commented Jan 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.13%. Comparing base (8842d37) to head (4e5dbb1).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #8968   +/-   ##
=======================================
  Coverage   84.13%   84.13%           
=======================================
  Files         120      120           
  Lines        8117     8119    +2     
  Branches     1948     1948           
=======================================
+ Hits         6829     6831    +2     
  Misses       1288     1288           

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

@@ -617,3 +620,7 @@ videojs.url = Url;
videojs.Error = VjsErrors;

export default videojs;
export {
Player,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This allows adding to the Player interface in plugins:

declare module 'video.js' {
  interface Player {
    touchOverlay: TouchOverlay;
    touchOverlayDirect: TouchOverlay;
    mobileUi: {
      (options?: MobileUiPluginOptions): void;
      VERSION: string;
    }
  }
}

* The key/value store of options that will get passed to children of
* the child.
*
* @param {number} [index=this.children_.length]
* The index to attempt to add a child into.
*
*
* @return {Component}
* @return {T}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renders like this in the API docs with the changes to jsdoc-typeof-plugin:
image

@mister-ben mister-ben changed the title [WIP] Type enhancements refactor(types): Type enhancements Feb 15, 2025
@mister-ben mister-ben marked this pull request as ready for review February 15, 2025 09:32
Comment on lines 6 to 15
const templateMatch = /@template {(.*)} ([A-Z])\n/.exec(event.comment || '');

if (templateMatch) {
const className = templateMatch[1];
const templateName = templateMatch[2];

event.comment = event.comment.replace(new RegExp(`{(.*\\b)(${templateName})(\\b.*)}`, 'g'), `{$1typeof ${className}$3}`);
}

// \{.*\bT\b.*\}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's possible to modify the regex so that this case is also handled?

    class CustomComponent extends Component{
      __z(){}
    }
    const Custom = Component.registerComponent('CustomComponent', CustomComponent);
    const custom = new Custom(player);

Instead of:
Screenshot from 2025-02-16 19-57-07

We would have:
Screenshot from 2025-02-16 19-58-05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! The latest commit should cover that.

Comment on lines 1986 to 1989
* @param {typeof Component} ComponentToRegister
* The `Component` class to register.
*
* @return {Component}
* @return {typeof Component}
Copy link
Contributor

Choose a reason for hiding this comment

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

Based on my previous comment, if the regex could handle @template {typeof Component} T we could have:

  /**
   * 
   * @template {typeof Component} T
   * 
   * Register a `Component` with `videojs` given the name and the component.
   *
   * > NOTE: {@link Tech}s should not be registered as a `Component`. {@link Tech}s
   *         should be registered using {@link Tech.registerTech} or
   *         {@link videojs:videojs.registerTech}.
   *
   * > NOTE: This function can also be seen on videojs as
   *         {@link videojs:videojs.registerComponent}.
   *
   * @param {string} name
   *        The name of the `Component` to register.
   *
   * @param {T} ComponentToRegister
   *        The `Component` class to register.
   *
   * @return {T}
   *         The `Component` that was registered.
   */

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

Successfully merging this pull request may close these issues.

HTMLTrackElement class provided missing two properties as stated in MDN
2 participants