Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

feat: narrow down type of currentTarget #142

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TylorS
Copy link
Member

@TylorS TylorS commented May 28, 2019

This PR tries to narrow down the type of currentTarget when the node type is statically known. I think that line 33 and down may be a breaking change because it makes the type of node no longer support all EventTargets but looking on MDN these are the places that implement these events. If it's preferred I can put that change into a separate PR to just get the currentTarget changes merged before.

@TylorS
Copy link
Member Author

TylorS commented May 28, 2019

I think we could also do this same thing for Flow right?

@briancavalier
Copy link
Member

briancavalier commented May 29, 2019

This looks really nice, @TylorS. I see what you're saying about lines 33 and below, but I have no idea how to gauge how painful it might be. I guess someone could be using a custom event or custom EventTarget.

I certainly don't mind doing a major version bump. One strategy would be to include the potentially breaking changes, bump major, and then if it turns out to be prohibitive, loosen the types in a subsequent patch or minor release. Thoughts? Other options?

As for Flow, yeah, I think the same approach should work. 👍.

@TylorS
Copy link
Member Author

TylorS commented May 29, 2019

Yeah, moving back to EventTarget afterwards shouldn't even a breaking change after the initial major bump. I think that's a pretty solid plan.

I can try to add the corresponding flow types soon 👍

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants