ysmood / yaku

A lightweight promise library
https://tonicdev.com/ysmood/yaku
MIT License
290 stars 28 forks source link

incorrect behavior with unhandledrejection events #46

Closed rtsao closed 7 years ago

rtsao commented 7 years ago

Per https://github.com/ysmood/yaku/blob/master/docs/debugHelperComparison.md#better-unhandled-error, Yaku will not fire an unhandledrejection event in this case:

p0 = Yaku.resolve().then(function () {
    abc()
})
p1 = p0.then(function () {})
p2 = p0.then(function () {})
p3 = p0.catch(function () {})

From my understanding, this is incorrect behavior. p1 and p2 are new promises that have unhandled rejections.

I think the following should happen:

window.onunhandledrejection = ({promise}) => { 
  console.log('window.onunhandledrejection: p' + promises.indexOf(promise));
}
p0 = Yaku.resolve().then(function () {
    abc()
})
p1 = p0.then(function () {})
p2 = p0.then(function () {})
p3 = p0.catch(function () {})
promises = [p0, p1, p2, p3]

// should log 'unhandledrejection: p1'
// should log 'unhandledrejection: p2'

I believe Chrome actually implements the spec correctly.

ysmood commented 7 years ago

However, Javascript is a dynamic language, for all the implementations unhandled rejection is just a guess, you never know if an async error is handled at some point in the future . For example, no implementation can handle the code below correctly:

var p = Promise.reject()

setTimeout(() => {
     p.catch(() => {})
}, 1e9)

Actually the vm needs to understand the meaning of your code to handle it correctly, which is apparently impossible at current AI tech.

So, the problem changes to make the guess warning less annoying. To suppress it as much as possible.

Your p1 andy p2 is just containers, they contain the same error value. Why we should waste cpu to log it twice?

ysmood commented 7 years ago

Another thing the spec doesn't mention is what is an unhandled rejection philosophically?

This code:

p0 = Yaku.resolve().then(function () {
    abc()
})
p1 = p0.then(function () {})
p2 = p0.then(function () {})
p3 = p0.catch(function () {})

Apparently has a handler for the original rejection.

But on the other side, you could also say it doesn't have a handler for the inherent rejections.

It really depends on your definition of unhandled rejection.

rtsao commented 7 years ago

However, Javascript is a dynamic language, for all the implementations unhandled rejection is just a guess, you never know if an async error is handled at some point in the future.

Right, which is why both unhandledrejection and rejectionhandled events are emitted. It's up to the user to listen to both and reconcile the two to determine the unhandled promise rejections. For example: https://googlechrome.github.io/samples/promise-rejection-events/index.html

Your p1 andy p2 is just containers, they contain the same error value. Why we should waste cpu to log it twice?

Because it's the spec. If I'm writing an error reporting framework, I want to build it on top of how the spec works (i.e. it works the the same with native promises and Yaku).

ysmood commented 7 years ago

I prefer the original rejection as I mentioned, it's more intuitive.

rtsao commented 7 years ago

Fair enough, feel free to close this issue if you disagree.

However, my personal opinion is that it should conform to the spec rather than use some special snowflake logic on how these events operate.

ysmood commented 7 years ago

What spec do you mean? unhandled rejection is not a part of ES spec, Chrome is not a spec. For me it's a runtime decision.

rtsao commented 7 years ago

It's not codified in ECMAScript, which only suggests a typical implementation for HostPromiseRejectionTracker:

A typical implementation of HostPromiseRejectionTracker might try to notify developers of unhandled rejections, while also being careful to notify them if such previous notifications are later invalidated by new handlers being attached.

However, I think browser unhandledrejection and rejectionhandled events are part of a codified browser spec (not just Chrome), which does dictate behavior. It is not up to the runtime (for browsers):

https://html.spec.whatwg.org/multipage/webappapis.html#unhandled-promise-rejections https://html.spec.whatwg.org/multipage/webappapis.html#the-hostpromiserejectiontracker-implementation

https://github.com/w3c/web-platform-tests/tree/master/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections

ysmood commented 7 years ago

That why I said Chrome doesn't handle it improperly here: https://github.com/ysmood/yaku/blob/master/docs/debugHelperComparison.md#better-unhandled-error

If I think they are all right, why would I take time to optimize the behavior? If I follow them, the codebase could be smaller.

I hope my thoughts could help to evolve the spec 😁