Skip to content
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

Event type for readystatechange is not specific enough #969

Closed
wants to merge 1 commit into from
Closed

Event type for readystatechange is not specific enough #969

wants to merge 1 commit into from

Conversation

jonhue
Copy link

@jonhue jonhue commented Jan 4, 2021

Fixes microsoft/TypeScript#41775

I was a little bit unsure whether its best to use ProgressEvent<Document> vs making Event generic (Event<T extends EventTarget = EventTarget>) and then using Event<Document>.

@jonhue jonhue requested a review from sandersn as a code owner January 4, 2021 12:46
@HolgerJeromin
Copy link
Contributor

This is a bit of a cherrypicking of all the unspecific typed events (even if you only look at document).
even document.onreadystatechange is not changed... 8-/

ref #899 and microsoft/TypeScript#299

@jonhue
Copy link
Author

jonhue commented Jan 5, 2021

@HolgerJeromin Thank you for these pointers!

I actually would be in favor of turning the Eventtype into something like Event<T extends EventTarget = EventTarget>.
That shouldn't really break existing code except altering some error messages.

But this would be a larger change, so without someone from the TypeScript team chiming in and telling us they are looking for something like that, I'm somewhat hesitant to implement it. At least as part of this pr.

@orta
Copy link
Contributor

orta commented Feb 23, 2021

Looking at all the past issues with trying to make these types more explicit, we're very wary of changes here which adds generics to types which are re-used all of the place. I know it's not a perfect representation of the underlaying DOM as it is, but we're making perf vs memory vs accuracy trade-offs and I think we'll probably not take this.

@jonhue
Copy link
Author

jonhue commented Feb 23, 2021

@orta makes sense :) should we then close the original issue, so that we don't spend more time on this in the future?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Event type for readystatechange is not specific enough
3 participants