[8.14.1] Type errors

[boris-petrov]

Description

Thanks for the new release! However, I believe there is an issue in the new types:

component.d.ts

ready(fn: (this: Component) => Component, sync?: boolean): Component;

The return type of the fn parameter is not supposed to be Component but rather void - I don't think we have to return values from the callback in the ready method, right?

Also, when called on the Player, isn't the this going to be the Player instance? As In that case the this argument is wrongly marked as Component:

Type 'Component' is missing the following properties from type 'Player': boundDocumentFullscreenChange_, boundFullWindowOnEscKey_, boundUpdateStyleEl_, boundApplyInitTime_, and 209 more.

Reduced test case

N/A

Steps to reproduce

Install video.js and use TS.

Errors

No response

What version of Video.js are you using?

8.14.1

Video.js plugins used.

No response

What browser(s) including version(s) does this occur with?

N/A

What OS(es) and version(s) does this occur with?

Linux

[gkatsev]

The ready method comes from Component, which Player inherits from. ready on a component, would have the this be that Component, which, in the case of Player, would be the player.
We don't override the method in player.js, but maybe we can use jsdoc to update the method signature there?

[boris-petrov]

I'm not sure, maybe overriding the type signature is possible. This is not that important if it's difficult to fix as users can cast the usage in the function so it's kind-of OK. The other problem with the return type is breaking all usages of ready though.

[gkatsev]

yeah, these two lines should be removed. Not sure why they were re-added the last time that piece was touched:

video.js/src/js/component.js

Lines 841 to 842 in 8729e31

	* @return {Component}
	* Returns itself; method can be chained.

[boris-petrov]

I guess that is wrong, yes, because I don't see the code returning anything from ready. But will that fix the other issue that the fn argument is marked as returning Component? I don't understand how this maps to the fn parameter? I see fn defined as a ReadyCallback above this but I don't find a definition for ReadyCallback anywhere in the codebase.

[mister-ben]

ReadyCallback is here, but I don't know why that is interpreted as it would return a Component

video.js/src/js/component.js

Lines 28 to 34 in 8729e31

	/**
	* 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
	*/

[boris-petrov]

Looking in VSCode, when I hover over ReadyCallback, the tooltip says type ReadyCallback = /*unresolved*/ any. I'm not sure how this @callback annotation works. But maybe because TS can't resolve ReadyCallback it thinks that the line below (which says @return {Component}) also applies to it? But on the other hand I see that fn is defined with this: Component so it did somehow read the definition of ReadyCallback... I'm not sure, all these jsdoc annotations are really strange. 😄

[mister-ben]

It's all very complicated because typescript understands a subset of jsdoc when generating the types, and also some typescript-specific syntax in jsdoc but which jsdoc itself doesn't understand, and breaks generating the API docs. Finding a balance that works for both is hard.

If I remove the @return from ready() and add @return {undefined} in Component.js, then component.d.ts has this:

ready(fn: () => undefined, sync?: boolean): void;

[boris-petrov]

This also doesn't work, the return type must be void. Doesn't @return {void} work? Or simply removing @return?

[mister-ben]

It looks like jsdoc is ok with using @param {function():void} fn ? The API docs output defines it as a function, and the type definition becomes ready(fn: () => void, sync?: boolean): void;. Seems simpler than defining ReadyCallback.

[mister-ben]

{function():void} seems to work by accident rather than design, and would break jsdoc if it were any more complicated, e.g. documenting arguments. So not a good precedent to set. Going back to defined callbacks, I have this, which specifies this for the Player's callback but leaves Component's unspecified. Returning {void} isn't really right in jsdoc itself as js functions that don't explicitly return do return undefined. But it's probably well understood. Any thoughts?

[boris-petrov]

I'm really not the person to review JSDoc. 😄 But as for the undefined - of course JavaScript itself doesn't have a concept of void but TypeScript does - and that's absolutely the right way to specify this function as it indeed does not return a value (even though there is an implicit return at runtime).