-
Notifications
You must be signed in to change notification settings - Fork 226
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
Update type definitions #219
Conversation
@lpinca, when you have time, can you please take a look at this PR? |
I'm not qualified to review this PR as I don't use TypeScript. cc: @delta62 |
@gfmio is this a breaking change or can it cause issues if released as a patch release? |
No, this won't break anything, so it can be released as a patch. The |
Ok. I'll merge it then. |
Thank you. |
Thanks a lot, @lpinca! |
Haven’t looked in detail yet but FYI looks like this breaks hlsjs. See video-dev/hls.js#2718 |
+1 |
Can you provide a backward compatible patch? Otherwise I will revert it. |
It seems the issue with hls.js has been addressed in video-dev/hls.js@48db1a5. |
@lpinca Having a look now |
@tjenkinson @teazean @lpinca I've reproduced the error. The typings on the default export are correct, but not on the named export. I will file a new PR to patch this. Thanks for reporting. |
Great thanks! |
@tjenkinson Can you verify that 4.0.3 fixes all issues for you? |
@gfmio 4.0.3 works fine again without needing |
thanks, 4.0.3 works fine. |
This PR adds a new set of type definitions to this library, as outlined in issue #217.
Right now, if the type parameter is a map of events and parameter array types, the parameters themselves don't get human-readable names. If the event map type instead supported using entire function annotations, the parameter names could be retained which makes using the library easier.
Before the changes in this PR, you could only do the following:
With the changes in this PR, you could do this:
Additionally, the PR adds an optional type parameter for the
context
object.