Playlist loader typescript #2144
Conversation
src/loader/playlist-loader.ts
Outdated
| @@ -332,27 +331,29 @@ class PlaylistLoader extends EventHandler { | |||
| }); | |||
| } | |||
|
|
|||
| _handleTrackOrLevelPlaylist (response, stats, context, networkDetails) { | |||
| _handleTrackOrLevelPlaylist (response: LoaderResponse, stats: LoaderStats, context: PlaylistLoaderContext, networkDetails: any) { | |||
There was a problem hiding this comment.
Type for networkDetails ?
There was a problem hiding this comment.
So, networkDetails can be a XHR object, but since loaders can be passed in via config and return "whatever" as network details and this just pipes it around, we can't actually guarantee at the type level what it is. Or at least that was my reason for leaving it as "any".
I think that one option would be to put the networkDetails in the Context, that way the loader can define what networkDetails it uses.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // the level index to load | ||
| level: number | null | ||
| // TODO: what is id? | ||
| id: number | null |
There was a problem hiding this comment.
I'm assuming this can be null only initially or if something is invalid.
In which case could we normalize this to be -1? That way the type is always number and we don't have to use the as keyword
There was a problem hiding this comment.
I'm not sure what the meaning of null was, but it could be valid to pick a better initalized value.
| // the level index to load | ||
| level: number | null | ||
| // TODO: what is id? | ||
| id: number | null |
There was a problem hiding this comment.
I'm not sure what the meaning of null was, but it could be valid to pick a better initalized value.
src/loader/playlist-loader.ts
Outdated
| @@ -332,27 +331,29 @@ class PlaylistLoader extends EventHandler { | |||
| }); | |||
| } | |||
|
|
|||
| _handleTrackOrLevelPlaylist (response, stats, context, networkDetails) { | |||
| _handleTrackOrLevelPlaylist (response: LoaderResponse, stats: LoaderStats, context: PlaylistLoaderContext, networkDetails: any) { | |||
There was a problem hiding this comment.
So, networkDetails can be a XHR object, but since loaders can be passed in via config and return "whatever" as network details and this just pipes it around, we can't actually guarantee at the type level what it is. Or at least that was my reason for leaving it as "any".
I think that one option would be to put the networkDetails in the Context, that way the loader can define what networkDetails it uses.
Per feedback from John, the further defined response type caused issue in callers.
src/loader/playlist-loader.ts
Outdated
| @@ -332,27 +331,29 @@ class PlaylistLoader extends EventHandler { | |||
| }); | |||
| } | |||
|
|
|||
| _handleTrackOrLevelPlaylist (response, stats, context, networkDetails) { | |||
| _handleTrackOrLevelPlaylist (response: LoaderResponse, stats: LoaderStats, context: PlaylistLoaderContext, networkDetails: any) { | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…js into playlist-loader-typescript
This PR will...
convert playlist loader to typescript.
Why is this Pull Request needed?
Progress towards typescripifying.
Are there any points in the code the reviewer needs to double check?
Loader interfaces similar enough to be used in: #2123
Resolves issues:
N/A
Checklist