Skip to content

Commit

Permalink
fix(types): fix and improve component ready callback definition (#8766)
Browse files Browse the repository at this point in the history
- Removes erroneous `@return {Component}` from `ReadyCallback` in
Component
- Remove `@this {Component}` from `ReadyCallback` in Component, because
this is not accurate for classes that extend Component
- Adds `@returns {void}` to `ReadyCallback` in Component
  - Produces a more accurate typedef
- Isn't strictly accurate in jsdoc/javascript but does this matter since
it's well understood. Absence of `@return` is interpreted by tsc as
returning `{any}`, `@returns {undefined}` would require an explicit
`return undefined` in ts.
- Adds a `{PlayerReadyCallback}` in Player with `@this {Player}` used in
the `new Player()` and `videos()` constructors.
  - Are we ok adding this new typedef
- Is inconsistent with `player.ready()` which uses `ReadyCallback`
without `@this` - but this can't be changed without adding an otherwise
unnecessary override just to pander to tsc.

## Requirements Checklist
- [x] Feature implemented / Bug fixed
- [ ] If necessary, more likely in a feature request than a bug fix
- [x] 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](https://codepen.io/gkatsev/pen/GwZegv?editors=1000#0))
- [x] Has no DOM changes which impact accessiblilty or trigger warnings
(e.g. Chrome issues tab)
  - [x] Has no changes to JSDoc which cause `npm run docs:api` to error
- [ ] Reviewed by Two Core Contributors
  • Loading branch information
mister-ben committed Jul 6, 2024
1 parent 6221a8f commit 970faa8
Show file tree
Hide file tree
Showing 3 changed files with 18 additions and 20 deletions.
19 changes: 8 additions & 11 deletions src/js/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,14 @@ import {merge} from './utils/obj.js';

/** @import Player from './player' */

/**
* A callback to be called if and when the component is ready.
* `this` will be the Component instance.
*
* @callback ReadyCallback
* @returns {void}
*/

/**
* Base class for all UI Components.
* Components are UI objects which represent both a javascript object and an element
Expand All @@ -25,14 +33,6 @@ import {merge} from './utils/obj.js';
*/
class Component {

/**
* A callback that is called when a component is ready. Does not have any
* parameters and any callback value will be ignored.
*
* @callback ReadyCallback
* @this Component
*/

/**
* Creates an instance of this class.
*
Expand Down Expand Up @@ -837,9 +837,6 @@ class Component {
*
* @param {ReadyCallback} fn
* Function that gets called when the `Component` is ready.
*
* @return {Component}
* Returns itself; method can be chained.
*/
ready(fn, sync = false) {
if (!fn) {
Expand Down
8 changes: 7 additions & 1 deletion src/js/player.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,12 @@ import './tech/html5.js';
/** @import { TimeRange } from './utils/time' */
/** @import HtmlTrackElement from './tracks/html-track-element' */

/**
* @callback PlayerReadyCallback
* @this {Player}
* @returns {void}
*/

// The following tech events are simply re-triggered
// on the player when they happen
const TECH_EVENTS_RETRIGGER = [
Expand Down Expand Up @@ -307,7 +313,7 @@ class Player extends Component {
* @param {Object} [options]
* Object of option names and values.
*
* @param {Function} [ready]
* @param {PlayerReadyCallback} [ready]
* Ready callback function.
*/
constructor(tag, options, ready) {
Expand Down
11 changes: 3 additions & 8 deletions src/js/video.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,8 @@ import xhr from '@videojs/xhr';
import Tech from './tech/tech.js';
import { use as middlewareUse, TERMINATOR } from './tech/middleware.js';

/** @import { PlayerReadyCallback } from './player' */

/**
* Normalize an `id` value by trimming off a leading `#`
*
Expand All @@ -53,13 +55,6 @@ import { use as middlewareUse, TERMINATOR } from './tech/middleware.js';
*/
const normalizeId = (id) => id.indexOf('#') === 0 ? id.slice(1) : id;

/**
* A callback that is called when a component is ready. Does not have any
* parameters and any callback value will be ignored. See: {@link Component~ReadyCallback}
*
* @callback ReadyCallback
*/

/**
* The `videojs()` function doubles as the main function for users to create a
* {@link Player} instance as well as the main library namespace.
Expand Down Expand Up @@ -121,7 +116,7 @@ const normalizeId = (id) => id.indexOf('#') === 0 ? id.slice(1) : id;
* Options object for providing settings.
* See: [Options Guide](https://docs.videojs.com/tutorial-options.html).
*
* @param {ReadyCallback} [ready]
* @param {PlayerReadyCallback} [ready]
* A function to be called when the {@link Player} and {@link Tech} are
* ready.
*
Expand Down

0 comments on commit 970faa8

Please sign in to comment.