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

"If an exception was propagated out from any event handler" is not a thing #140

Closed
annevk opened this issue Jan 21, 2017 · 9 comments
Closed
Labels
Milestone

Comments

@annevk
Copy link
Member

annevk commented Jan 21, 2017

While reading through #87 I found Indexed DB has been monkey patching DOM as well with something that is not supported by the event dispatch algorithm. The event dispatch algorithm handles its own exceptions and reports them to window.onerror. It seems you're trying to hook into that somehow.

@annevk
Copy link
Member Author

annevk commented Jan 21, 2017

In particular in 7.5 of https://w3c.github.io/IndexedDB/#upgrade-transaction-steps.

@inexorabletash
Copy link
Member

Nicely spotted. This has been in the spec since forever (early iterations of v1) and behavior is consistent across browsers, so we may need another hook unless someone can come up with a clever way of describing this.

(Hopefully there aren't too many more monkey patches lurking?).

@annevk
Copy link
Member Author

annevk commented Jan 22, 2017

Does the error still go to window.onerror? We don't really have a concept of UA-event-listeners, so I suspect we need some kind of hook.

@inexorabletash
Copy link
Member

I haven't spec'd window.onerror integration yet (see #49) - it's implemented in Firefox but not Chrome. (I hit a performance regression whenever I try, and then get distracted by other shiny things.) Since IDB is exposed in workers there was apparently some trickiness on the spec side too, but that may have been sorted out.

But in Firefox, yes - an error from a request that does not have preventDefault() called on the event causes the transaction to abort and propagates to window.onerror, but by the time it hits the global error handler it's too late to "preventDefault()" and stop the transaction from aborting.

@annevk
Copy link
Member Author

annevk commented Jan 23, 2017

If you call preventDefault() and then throw, it would not abort? That doesn't seem to be specified. But I guess the cleanest integration would be to change how dispatch works. Either we need to return more information than just a boolean, or you need to pass something in that gets modified (which is somewhat ugly I think, but okay).

@inexorabletash
Copy link
Member

Sorry, was mostly asleep - I was thinking about the error event case, not the exception case. I'll do some testing about preventDefault() vs. throwing and report back.

And either approach (additional return value or an "out" parameter or callback) is fine with me.

@inexorabletash
Copy link
Member

Oh yes, needed to follow up here.

PR for tests added. As expected, preventDefault() doesn't change behavior. And behavior is the same across event handlers/listeners, and (in the case of error) how far the event has bubbled.

annevk added a commit to whatwg/dom that referenced this issue Feb 14, 2017
Indexed Database API could have done with some architectural design
review.

See w3c/IndexedDB#140 for context.
@annevk
Copy link
Member Author

annevk commented Feb 14, 2017

I created a hook for the "dispatch" algorithm that you can use. (And review!)

annevk added a commit to whatwg/dom that referenced this issue Mar 6, 2017
Indexed Database API could have done with some architectural design
review.

See w3c/IndexedDB#140 for context.
@inexorabletash
Copy link
Member

Resolved in 214a135

Thanks all!

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

2 participants