PromiseRejectionEvent differs from Living Standard spec

[abirmingham]

1. What version of bluebird is the issue happening on?
   3.5.0

2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)
   Google Chrome 56.0

3. Did this issue happen with earlier version of bluebird?
   Yes.

Bluebird seems to be creating a PromiseRejectionEvent with a 'detail' property, which contains 'promise' and 'reason' properties. TypeScript complains that PromiseRejectionEvent has no 'detail' property, which prompted me to look at the specification for this event. It appears that 'promise' and 'reason' are intended to be properties of the event object itself, e.g. event.promise and event.reason.

Perhaps I'm missing something?

Spec info found here -
https://developer.mozilla.org/en-US/docs/Web/API/PromiseRejectionEvent/PromiseRejectionEvent
https://html.spec.whatwg.org/multipage/webappapis.html#promiserejectionevent

[benjamingr]

Bluebird fired these events for several years before browsers specified them so it uses a version that predates what the DOM settled on.

Note that you can use onPossiblyUnhandledRejection on the library itself and re-emit the events as you see fit if you'd like.

[abirmingham]

That makes sense! Thanks for the explanation.

Now that a standard has been defined, shouldn't bluebird adhere to it? It seems like it would be worth the breaking change as part of a major release.

[petkaantonov]

It can be done in backwards compatible way

[nornagon]

Bugsnag was having trouble picking up on Bluebird's unhandled rejection messages (see https://github.com/bugsnag/bugsnag-js/blob/master/src/bugsnag.js#L1285), hopefully #1464 fixes this!

[benjamingr]

PR LGTM, @domenic can you please take a look and confirm this is compatible with the standard?

[domenic]

I mean it's not exactly an implemenation of it, and differs in little ways (e.g. getter vs. data property) but if the goal is to be able to write code that works roughly the same in both, adding promise and reason properties to the event object seems reasonable.

[benjamingr]

I mean it's not exactly an implemenation of it, and differs in little ways (e.g. getter vs. data property) but if the goal is to be able to write code that works roughly the same in both, adding promise and reason properties to the event object seems reasonable.

If we're doing this (which I think we should to ensure an easy transition from/to bluebird) I think we should actually implement the differences. Is there anything we can test against? (as in, are there tests for how the event is fired we can make sure we pass?)

@nornagon would you be willing to change the code to getters?

[nornagon]

See #1464 (comment), would that be a better way of implementing this?

[nornagon]

Updated the PR to use getters.

[domenic]

I think we should actually implement the differences

I don't feel that's realistic for a Node.js API? E.g. there's no Event class in Node.js, so you'll have to implement that whole hierarchy. Then you'll have to get rid of the detail property you're currently using. There's a lot more...

(as in, are there tests for how the event is fired we can make sure we pass?)

The tests were largely derived from the Node.js tests, but they live at https://github.com/w3c/web-platform-tests/tree/master/html/webappapis/scripting/processing-model-2/unhandled-promise-rejections

[elad-maimon]

Hello,

Why was this issue closed?
The suggestion above of using onPossiblyUnhandledRejection doesn't seem to be aligned with documentation that says

this hook is specific to the bluebird instance its called on, application developers should use global rejection events

Also, typescript would not allow solutions like that one that was mentioned above, unless we have some type to define the event that bluebird fires (searched in DefinitelyTyped and couldn't find).

IMHO Bluebird should be aligned with the spec and trigger the event with PromiseRejectionEvent instance, for now, if there's type definition it could be really helpful.

Thanks!

[benjamingr]

IMHO Bluebird should be aligned with the spec and trigger the event with PromiseRejectionEvent instance, for now, if there's type definition it could be really helpful.

The local unhandled rejection events and the global ones are separate and solve separate things.

The last batch of differences was fixed in 8991667

This issue is misleading because it broke again before it was fixed.

[benjamingr]

It's possible we haven't had a release with this yet because of the async_hook fears. @petkaantonov ?