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

PromiseRejectionEvent does not conform to spec again #1509

Closed
l1bbcsg opened this issue Mar 14, 2018 · 15 comments
Closed

PromiseRejectionEvent does not conform to spec again #1509

l1bbcsg opened this issue Mar 14, 2018 · 15 comments

Comments

@l1bbcsg
Copy link

@l1bbcsg l1bbcsg commented Mar 14, 2018

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

  2. What platform and version? (For example Node.js 0.12 or Google Chrome 32)
    Testing in Chrome 67, but it's irrelevant

  3. Did this issue happen with earlier version of bluebird?
    Do not know

Event objects fired with unhandledrejection and rejectionhandled do not conform to PromiseRejectionEvent specification: They do not have reason and promise properties, instead they have detail property which contains the two.

This issue was already raised as #1447 and fixed in #1464
And evidently the code is still there in master.

But it's gone in the build.
Here's the snipped from bluebird.core.js as compiled by default and as linked on the site (https://cdn.jsdelivr.net/bluebird/latest/bluebird.core.js):

		var fireDomEvent = (function() {
			try {
				if (typeof CustomEvent === "function") {
					var event = new CustomEvent("CustomEvent");
					util.global.dispatchEvent(event);
					return function(name, event) {
						var domEvent = new CustomEvent(name.toLowerCase(), {
							detail: event,
							cancelable: true
						});
						return !util.global.dispatchEvent(domEvent);
					};
				} else if (typeof Event === "function") {
// ...

For some reasons, property defines are gone.
I imagine this is either issue with build scripts or es5.defineProperty shim.

@benjamingr
Copy link
Collaborator

@benjamingr benjamingr commented Mar 14, 2018

Thanks for the report.

I think it's possible we simply haven't updated the core build in a while - can you try building yourself?

@l1bbcsg
Copy link
Author

@l1bbcsg l1bbcsg commented Mar 14, 2018

This seems to be the case.

Contents of the package in registry (https://registry.npmjs.org/bluebird/-/bluebird-3.5.1.tgz) have precompiled artefacts in js/browser directory without those defines. Therefore installing bluebird as a dependancy of another package yields some apparently outdated version which is still named 3.5.1 in package.json and in comment header in the file itself.
Same with the version linked on the website.

Cloning the repo and running build scripts (well, they run automatically through prepublish hook) yields a version with those defines (which is also named 3.5.1 both in package.json and in comment header).

Diff between the two files shows the PromiseRejectionEvent properties, some FakeConstructor magic and copyright year.

@l1bbcsg
Copy link
Author

@l1bbcsg l1bbcsg commented Mar 15, 2018

Upon further investigation, it seems there's another issue atop the one with wrong build. Emitted events still do not have the necessary properties even with correct build.

According to CustomEvent WHATWG specification the constructor will ignore properties of the second CustomEventInit parameter that are not part of the Event interface. As such, detail and promise never actually get added to CustomEvent.

let data = { cancelable: true, foo: 'foo' })
let event = new CustomEvent('test', data)

event.cancelable    // true
event.foo    // undefined

brenthompson added a commit to brenthompson/electron-unhandled that referenced this issue Apr 9, 2018
Bluebird.js promises don't have event.reason, they instead have event.detail.reason. An unhanded rejection from bb causes an exception from your plugin. Given Bluebird's popularity it's probably worthwhile handing this case at least until they fix their side. As you can see they've taken a couple of kicks at this already. Links to their issues:
petkaantonov/bluebird#1447
petkaantonov/bluebird#1509
@Whoaa512
Copy link

@Whoaa512 Whoaa512 commented Apr 12, 2018

Looks like this is not due to build issues. The commit which adds the property defines was merged on Oct 6, 2017 and 3.5.1 was released on Oct 4, 2017

@petkaantonov or @benjamingr would you mind releasing a patch version to get this updated?

@MadLittleMods
Copy link

@MadLittleMods MadLittleMods commented Apr 26, 2018

👍 on new release

We were getting a lot of undefined messages in Sentry(because e.reason is undefined) and worked around it here, https://gitlab.com/gitlab-org/gitter/webapp/merge_requests/1131

@genne
Copy link

@genne genne commented Jun 29, 2018

Any plans on doing a new release to fix this issue?

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented Sep 3, 2018

@l1bbcsg
Copy link
Author

@l1bbcsg l1bbcsg commented Sep 4, 2018

Could you also take a look at this: #1509 (comment)?

This might still be preventing Bluebird from generating proper events.

@seansfkelley
Copy link

@seansfkelley seansfkelley commented Jan 23, 2019

Just upgraded to 3.5.3 to attempt to fix this issue, but per #1509 (comment) the expected fields are not present on the CustomEvent.

Tooling around in my dev console, it looks like you can (indeed, must) provide at least promise and optionally reason to the PromiseRejectionEvent constructor:

> new PromiseRejectionEvent("unhandledrejection", {})
Uncaught TypeError: Failed to construct 'PromiseRejectionEvent': required member promise is undefined.
    at <anonymous>:1:1

> new PromiseRejectionEvent("unhandledrejection", { promise: Promise.reject()})
PromiseRejectionEvent <...>

> new PromiseRejectionEvent("unhandledrejection", { promise: Promise.reject()}).promise
Promise <...>

Perhaps using/polyfilling PromiseRejectionEvent is the proper way to do this? I'm using Chrome 71.0.3578.98.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented May 25, 2019

@seansfkelley they are on the detail object

e.detail.reason and e.detail.promise

see http://bluebirdjs.com/docs/api/error-management-configuration.html#global-rejection-events

@seansfkelley
Copy link

@seansfkelley seansfkelley commented May 25, 2019

What I was trying to say with that comment is not that the values are missing entirely, it's that like the OP says they're present, but in the wrong place:

They do not have reason and promise properties, instead they have detail property which contains the two.

Which your comment and the docs corroborate. This appears to still be the case in 3.5.5; on Firefox 66.0.5 (macOS 10.14) I get the following behavior:

> window.addEventListener('unhandledrejection', function(e) { console.log(e.reason, e.promise, e.detail.reason, e.detail.promise); });
> Promise.reject('foo');
undefined undefined foo Object { ... }

It also appears to be the case on 3.5.2, which suggests that this issue was never fixed in the way that the OP was requesting.

@l1bbcsg
Copy link
Author

@l1bbcsg l1bbcsg commented May 28, 2019

There are actually two issues: First that they're in the wrong place, second is that they are missing (from both places).

First was meant to be fixed in #1464 and then again in this issue.

The second was never addressed. See #1509 (comment) – it is simply not possible to have these properties added to CustomEvent.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented May 28, 2019

Maybe they be added as ad hoc properties?

var e = new Custo,mEvent(...);
e.promise = promise;
e.reason = reason

@seansfkelley
Copy link

@seansfkelley seansfkelley commented May 28, 2019

I don't know what the polyfill situation is, but PromiseRejectionEvent (when present) does work as intended as noted in #1509 (comment). Ad-hoc properties on CustomEvent seem like they should also serve.

@petkaantonov
Copy link
Owner

@petkaantonov petkaantonov commented May 29, 2019

Yes we need to keep .detail working as well to not break old code relying on it.

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

Successfully merging a pull request may close this issue.

None yet
7 participants