Skip to content

Conversation

devanshj
Copy link

@devanshj devanshj commented Nov 20, 2021

Following up from #44 (comment)
Supersedes #44

(Following up from #44 (comment)) This PR has one downside: with bindAll if you bind events "foo" and "click" then you'd get events Event and Event instead of Event and MouseEvent, that is to say if any one event is untyped then all elements become untyped. Test case.

There is another downside (which I think isn't exclusive to my approach) is: If you bind a target A | B, both A and B have events "foo" but their event types are different namely AFoo and BFoo, in such case the binding.listener silently (even if noImplictAny is on) becomes any instead of (event: AFoo | BFoo) => void. Test case. This case would be super super rare though.

PR has my standards/opinions with them ;) feel free to keep/change/remove whatever you want. Also feel free to ask questions.

@alexreardon
Copy link
Owner

I will try to take a look at this soon 👍

@alexreardon
Copy link
Owner

alexreardon commented Dec 3, 2021

Given the tradeoffs, do you think this is the way to go? Or stick with #44 ?

@devanshj
Copy link
Author

devanshj commented Dec 3, 2021

As I asked here, in #44 you can't do things like bind({} as EventTarget, { type: "customEvent", listener }) which is a breaking change. Are you fine with that?

If yes then then I'll update this PR to not allow untyped events like #44 does and then this will have no downsides and will be better than #44

If no, then #44 gets rejected as it is now, and we're left with this and I'll let you decide if it's this is better than status quo. This PR trades off the experience when involving untyped events in bindAll for completions for typed events in both bind and bindAll.

Also consider this less of a PR and more of a reply to @Andarist as he asked me how to go about it :P so I'd want to know what he thinks and his approval too. (No hurries I don't mind keeping this open for long)

@alexreardon
Copy link
Owner

Sorry for not actioning this sooner. I appreciate the time you put into this. Things have been super busy for me lately. I hope to get to this PR when I have the right mental energy early next year. Merry Christmas!!

@devanshj
Copy link
Author

Don't worry about it Alex, I understand. It was almost no work for me anyway haha, I don't mind any amount of delay. Merry Christmas!

@devanshj
Copy link
Author

Let's reopen this if @Andarist wants, closing for now.

@devanshj devanshj closed this Jan 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants