refactor: remove usage of any for networkDetails; #7458
Conversation
|
I'm not sure why the precommit hooks didn't fix up everything to match the prettier settings (i'll handle it on the next commit), but there are some issues with the types in the unit tests like this: Which I need to fix up now as well. |
Apologies for that. There's either an expectation that the IDE you use would be configured to run it on save, or you'd perform
Let me know if I can help. Thanks for picking this up again 😄 |
|
Unit tests are working!
I added a couple of inline comments on the PR to the two places I thought maybe the reason could be slightly unclear about why I did something. I think this is good to review, the missing checks seem to require credentials I won't have access to as an outside contributor. |
src/loader/playlist-loader.ts
Outdated
| if (response) { | ||
| const url = networkDetails?.url || context.url; | ||
| let url = context.url; | ||
| if (networkDetails instanceof Response) { |
There was a problem hiding this comment.
I'm not sure if we support any devices that are old enough that Response can be undefined? https://caniuse.com/?search=Response
There was a problem hiding this comment.
We should avoid this for devices running older (Safari 8 like) WebKit runtimes (Smart TV, and Android 4).
There was a problem hiding this comment.
How about this?
| if (networkDetails instanceof Response) { | |
| if ('url' in networkDetails) { |
There was a problem hiding this comment.
it will have to be
if (networkDetails && 'url' in networkDetail)
to safely handle 'url' in null or else we will get Uncaught TypeError: right-hand side of 'in' should be an object, got null
This is definitely a safer check on older devices so I will fix that up!
robwalch
left a comment
robwalch
left a comment
There was a problem hiding this comment.
This is great work! If it's not too much trouble, please roll back the test string ws changes, and remove the instanceof Response reference just in case. Thank you.
src/loader/playlist-loader.ts
Outdated
| if (response) { | ||
| const url = networkDetails?.url || context.url; | ||
| let url = context.url; | ||
| if (networkDetails instanceof Response) { |
There was a problem hiding this comment.
How about this?
| if (networkDetails instanceof Response) { | |
| if ('url' in networkDetails) { |
|
Hi @ondreian, Sorry but the last patch or two introduced branch conflicts. Can you resolve them? Let me know if you are not available to and I'll make an intermediate branch to address this in. Thank you! |
|
I can do this tomorrow morning hopefully |
github-project-automation
bot
moved this to Top priorities
in HLS.js Release Planning and Backlog
2 participants
This PR will...
Properly declare the type for
NetworkDetailsWhy is this Pull Request needed?
Removes reliance on
anytype in favor of properly declaringNetworkDetailsfor better developer ergonomics when matching against HTTP outcomesAre there any points in the code the reviewer needs to double check?
I traced the usage of
NetworkDetailsas best as possible to all existing usages in the codebase and found that in some cases it is declared asnetworkDetails?type and sometimes is is passed as anullobject. I did not clean this up because technically it could be a breaking change, but converging it so thatnetworkDetails?is preferred would make sense for code clarity and overall maintainability in the future.New Work Relative to Previous PR
There were several places where meaningful drift had occurred relative to my previous PR, most notably the new
instanceof Responsecheck to handle theurlproperty in theplaylist-loader.ts:https://github.com/video-dev/hls.js/compare/master...flowplayer:hls.js:refactor/nullable-network-details?expand=1#diff-9da3ee2cb4fcb3e4fb98d46740cd4d9e071fa77e4b26336c0750ac2456945fb5R667-R670
I felt the differences were enough to warrant a separate PR with all of the new interstitial loading logic and such, it wasn't a simple rebase and fixup.
Resolves issues:
resolves unnecessary dependence on
anytypeChecklist