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

Set default options for dispatchCustomEvent #126

Open
fbring opened this issue Jul 16, 2019 · 3 comments
Open

Set default options for dispatchCustomEvent #126

fbring opened this issue Jul 16, 2019 · 3 comments

Comments

@fbring
Copy link
Contributor

fbring commented Jul 16, 2019

Explain the problem

The usage of dispatchCustomEvent accepts 3 arguments:

  1. eventName
  2. details
  3. options

I think that in almost every case dispatchCustomEvent will be used for communicating with a parent component and therefore we need to set the bubbles: true option manually.

I searched in our current codebase for usage of dispatchCustomEvent:

  • 6/6 use bubbles: true
  • 4/6 use cancelable: true

Expected Behaviour

I would like to see at least the bubbles: true as a default option. Maybe it makes sense to set cancelable: true as well.

Actual Behaviour

Set all options manually inside the options object.

@byara
Copy link
Contributor

byara commented Jul 16, 2019

In most cases, we want this behavior, but isn't that a breaking change?

@pago
Copy link
Contributor

pago commented Jul 22, 2019

I'm wondering whether we really should be overwriting the DOM here. Making bubbles explicit has some benefits for readability (you clearly communicate the intend). I'd have a hard time to identify any valid use case for a non-bubbling event. So you could also say its a bugfix.
Theoretically bubbles: false might be useful for reusable ref-handlers but the ergonomics of those are currently not good enough so I doubt anybody uses that.

=> To me it's a bugfix. bubbles: false is very unlikely to have the expected behaviour and is most likely a cause of bugs. @byara do you agree?

What I do know for sure is that we shouldn't make cancelable a default. I doubt that we have a single line of code dealing with an event that has been cancelled. And if you don't deal with that case then there's no sense in marking an event as cancelable.

@byara
Copy link
Contributor

byara commented Jul 26, 2019

@pago agreed. I cannot come up with a case that I would set the bubbles: false.

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

No branches or pull requests

3 participants