-
Notifications
You must be signed in to change notification settings - Fork 2.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore: migrate NetworkDetails from any -> null|XMLHttpRequest|Response; #3828
base: master
Are you sure you want to change the base?
chore: migrate NetworkDetails from any -> null|XMLHttpRequest|Response; #3828
Conversation
| null // this is a nullable field in several callback interfaces | ||
| Response // from utils/fetch-loader.ts | ||
| XMLHttpRequest; // from utils/xhr-loader.ts |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for these changes. This should be helpful for anyone using the default loaders, and for improving internal type safety.
In a situation where someone provides their own loader via config.loader
(or pLoader
or fLoader
) and they want to provide their own struct to networkDetails
I wonder how using the project type exports will impact them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point, perhaps I need to expand the callback interface to be expressed with some generics and have the default argument be NetworkDetails
here.
Another option is to keep the current definition and say that a pLoader
or fLoader
must return one of these 3 types, in case we ever need to manipulate it internally we can have a verified interface versus a generic (that we can never verify like that), and they can cast Response | XMLHttpRequest
to their custom struct in their provided callback and outside of the hls.js internals
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use type imports where possible.
As for the build failing, my best guess is something about these changes would result in a type-check error in TS 4.1 (can't imagine what) and since that is what api-extractor (for which there is no update) is using, it errors silently). We could try making the api-extractor run verbose temprorrily. I don't know if we can force a new TS peer dependency to be used. Let's see if type imports mak a difference first (we should be using them anyway).
This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
@@ -0,0 +1,4 @@ | |||
export type NetworkDetails = | |||
| null // this is a nullable field in several callback interfaces |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would prefer to see NetworkDetails | null
where nullable.
This PR will...
Properly declare the type for
NetworkDetails
Why is this Pull Request needed?
Removes reliance on
any
type in favor of properly declaringNetworkDetails
for 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
NetworkDetails
as 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 anull
object. 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.Resolves issues:
resolves unnecessary dependence on
any
typeChecklist