Skip to content

Event handler removed after callback fails #1573

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

Closed
niahoo opened this issue Jul 1, 2018 · 4 comments
Closed

Event handler removed after callback fails #1573

niahoo opened this issue Jul 1, 2018 · 4 comments
Labels

Comments

@niahoo
Copy link

niahoo commented Jul 1, 2018

Hello,

I found what I think is a bug.

I listen to windows keyup events in a child component, so I can add and remove this component in a #if block (because my component is kind of a lightbox slideshow so most of the time it is not visible, so I want the event listeners to be removed).

In the parent component, I listen to the events fired by the child and handle them with a method.

What I found is that if the method throws an exception, the event handler of the parent will not be called anymore, but the child one still will be.

Here is an REPL demo.

Steps to reproduce :

  1. Go to the REPL and open your console (I tried chrome and Firefox, last stable ones)
  2. When loaded, click on the rendered area so the editor will not swallow keyboard events
  3. Push keys on your keyboard, but NOT the ArrowLeft key. You will see that the rendered area show what key you pushed. Look at the console, you will see logs from both components.
  4. Push the ArrowLeft key, you will see an exception in the console.
  5. Push keys on your keyboard, the View is not refreshed. Look at the console, you will see that the child component is handling window events, but not the parent component.

I have a repository too.

thank you

@Rich-Harris Rich-Harris added the bug label Jul 1, 2018
@Conduitry
Copy link
Member

What's happening here is the __calling flag on the handler is getting set to true, and then never set to false again because of the exception, and so the this.fire(...) doesn't actually call any more handlers for this event.

In other areas, Svelte has taken the position that not having unhandled exceptions is the developer's responsibility. There's also the question of what to do with other handlers that would have been called after the one that threw the exception. We don't want to just silently swallow the exception.

I guess my proposal of how to deal with this situation is; If a handler throws an exception, make sure we set __calling back to false, but then re-throw the exception from within fire and don't continue with the rest of the handlers.

@niahoo
Copy link
Author

niahoo commented Jul 9, 2018

Agreed, this seems the way native events work too.

Conduitry added a commit that referenced this issue Jul 9, 2018
* in .fire always set calling flag back to false (#1573)

* update expected bundles
@Conduitry
Copy link
Member

Fixed in 2.9.3 - thanks!

@niahoo
Copy link
Author

niahoo commented Jul 9, 2018

Cool, thank you :)

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

No branches or pull requests

3 participants