Type define the root hls.js #2200
Conversation
Also configured the TS compiler to emit declaration files to `dist/declarations`. Also, for a canary build this includes package.json `"types"` key to point at the declarations. This should be removed before merging this branch as we do NOT want to ask regular users to use our typing data yet until it is more complete than DT.
itsjamie
force-pushed
the
typescript-publicapi
branch
from
5586232
to
1085713
Compare
|
Also we could add the |
Yeah, I had it in there and removed it because the types aren't better than the DefinitelyTyped typings yet, so I don't want to make the usage harder until they're more complete. |
Implementing PR feedback from tjenkinson. Co-Authored-By: Tom Jenkinson <tom@tjenkinson.me>
|
I either have to stop doing stuff on Windows or take the time to figure out how to properly set up a Windows dev environment. Little things like lint exploding with CRLF errors and the like.. :( |
|
Let me know if there is anything keeping this out! |
| } | ||
|
|
||
| /** | ||
| * @type {QualityLevel[]} | ||
| */ | ||
| get levels () { | ||
| // todo(typescript-levelController) |
There was a problem hiding this comment.
HI @itsjamie,
I just started working on this in the jwplayer fork (LHLS branch) jwplayer/hls.js@LHLS...jwplayer:feature/typescript-level-and-track-controllers#diff-b071dfbf3f116515dee94f5b36c35a2aR16
I think it's a great idea to actually start with these changes since the hls player instance is referenced in all controllers, and we can decide whether certain getters should be nullable or return -1 in the case of number getters.
It seems that -1 was the default for most number getters until maybe recently. When I set up an Hls instance without loading anything, the results are mixed:
Any thoughts, preferences or concerns?
There was a problem hiding this comment.
I think if possible documenting current state would be helpful as TypeScript for the users who don't want to move to a 1.0.0 with the breaking changes. This is definitely seperate from the work I know you guys have done at JWPlayer, and I wouldn't ask you to change yours to document current state that won't be valid soon. I also wouldn't want to hold up progress just to add typing when the refactor already exists, so I think this concern is invalid.
@johnBartos @michaelcunningham19 @tjenkinson What do you guys think about targeting 1.0.0 as the cut-off for when we start publishing TypeScript library files from hls.js?
If we do the above ^, then it comes down to a decision of what do we want the API to be. It also means we don't have to worry about the intermittent state for many of the controllers @robwalch.
if (hls.subtitleTrack) { }, or if (hls.subtitleTrack !== -1) or some variant of this with a set const?
There was a problem hiding this comment.
I've covered more ground jwplayer/hls.js@LHLS...jwplayer:feature/typescript-level-and-track-controllers
firstLevel (showing NaN above) is the only getter I've changed the default for by giving it an initial value and strongly typing it. We have to be careful with some properties like startLevel which is also a config value where both undefined and -1 already mean something different (default first in manifest vs auto abr selection).
I'm not looking to shake things up in our fork or do any major refactoring with the typing here - no initially. The only changes would be when there are clearly errors, or it's clear that stronger typing benefits us. There's no point in just slapping any everywhere and calling it typed. Further down the line I'd want to look more carefully at what is nullable, optionally assigned or allowed to be undefined to see how to reduce the risk associated with transient properties.
The npm run build:types runs tsc with the CLI flag on.
itsjamie
dismissed stale reviews from johnBartos and michaelcunningham19
via
6ddd214
|
John confirmed in Slack he had tested the IE concerns and it was good. |
This PR will...
Add Type definitions to the hls.js file. 馃巻
Why is this Pull Request needed?
Starting to work outside-in for Typescript definitions.
Are there any points in the code the reviewer needs to double check?
There was a single actual behaviour change:
(config.liveSyncDuration === void 0 || config.liveMaxLatencyDuration <= config.liveSyncDuration)swapped from
(config.liveMaxLatencyDuration <= config.liveSyncDuration || config.liveSyncDuration === void 0)I don't think there is ever a reason to check the undefined case second?
Resolves issues:
No, but work toward #2070.
Checklist