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

fix unhandledrejection_event / onunhandledrejection edge compatibility #9350

Merged
merged 2 commits into from
Mar 10, 2021

Conversation

dgoldstein0
Copy link
Contributor

@dgoldstein0 dgoldstein0 commented Mar 6, 2021

The current data says it's available in <= 79. this conflicts with caniuse (https://caniuse.com/?search=unhandledrejection) as well as not making much sense and also conflicting with my own testing - I verified it's available in Edge 88; and it's also been in Chrome for a while. As 79 is the first Edge based on the same code base as Chrome, it seems far more likely that Edge >=79 is the correct range of Edge versions with support - or certainly more correct than the <=79 range.

My test in edge was basically

window.addEventListener("unhandledrejection", (e) => console.log("unhandled rejection handler fired"));
Promise.reject("error")

The catch is that the rejected promise needs to be executed in the web page - if you reject a promise in the console, it doesn't trigger the unhandledrejection handler. The exception handler however can be set in the console.

and then saw the text console.log'ed

The current data says it's available in <= 79.  this conflicts with caniuse (https://caniuse.com/?search=unhandledrejection) as well as not making much sense and also conflicting with my own testing - I verified it's available in Edge 88; and it's also been in Chrome for a while.  As 79 is the first Edge based on the same code base as Chrome, it seems far more likely that Edge >=79 is the correct range of Edge versions with support - or certainly more correct than the <=79 range.
@github-actions github-actions bot added the data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API label Mar 6, 2021
@dgoldstein0 dgoldstein0 changed the title fix unhandledrejection_event edge compatibility fix unhandledrejection_event / onunhandledrejection edge compatibility Mar 6, 2021
Copy link
Collaborator

@foolip foolip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can confirm this by testing Edge 18 and 80 with https://mdn-bcd-collector.appspot.com/tests/api/Window/onunhandledrejection. (I don't have access to Edge 79, but given this was added in Chrome 49, it's a very safe assumption to make.)

@foolip
Copy link
Collaborator

foolip commented Mar 10, 2021

Thanks for contributing this fix @dgoldstein0, and welcome to BCD!

Just to explain what ≤79 even meant, it means that it was added in Edge 79, but we're not sure if it was also supported in some earlier version. So the previous data was effectively "don't know" for Edge 12-18, and "supported" for Edge 79 and later.

A lot of these "ranged versions" are the result of bulk conversions, and any that can be turned into a "real version" as you've done here would be great to fix. I think that if you search for "≤79" in particular you will find quite a lot of things that weren't in Edge 18, but verifying it and sending PR takes time. Thanks for doing so here!

@foolip foolip merged commit 3832fa6 into mdn:master Mar 10, 2021
@dgoldstein0 dgoldstein0 deleted the patch-1 branch March 10, 2021 20:12
@dgoldstein0
Copy link
Contributor Author

ah, thanks for the explanation. I was definitely misinterpreting that to mean it wasn't supported after 79, which didn't seem right.

@foolip
Copy link
Collaborator

foolip commented Mar 10, 2021

@dgoldstein0 yeah I can see how https://developer.mozilla.org/en-US/docs/Web/API/Window/unhandledrejection_event#browser_compatibility makes it seems like was only supported in 79 and earlier, which is the opposite of what it's intended to convey.

@ddbeck @Elchi3 FYI that there are other interpretations of the range values than intended. Perhaps you've seen this before, but I hadn't. It's a reasonable interpretation, not sure if there's a fix other than getting rid of as many range versions as possible?

@Elchi3
Copy link
Member

Elchi3 commented Mar 11, 2021

@ddbeck @Elchi3 FYI that there are other interpretations of the range values than intended. Perhaps you've seen this before, but I hadn't. It's a reasonable interpretation, not sure if there's a fix other than getting rid of as many range versions as possible?

Unfortunately it came up a few times. Some improvements to the rendering on MDN were made, but yes, ideally, we only use ranged versions sparingly.

@ddbeck
Copy link
Collaborator

ddbeck commented Mar 14, 2021

@ddbeck @Elchi3 FYI that there are other interpretations of the range values than intended. Perhaps you've seen this before, but I hadn't. It's a reasonable interpretation, not sure if there's a fix other than getting rid of as many range versions as possible?

Unfortunately it came up a few times. Some improvements to the rendering on MDN were made, but yes, ideally, we only use ranged versions sparingly.

Maybe we could revisit it again? Maybe transform into ? on MDN?

@foolip
Copy link
Collaborator

foolip commented Mar 14, 2021

Since all of the allowed ranged versions are pretty far in the past now, I wonder if just treating them an exact version on MDN wouldn't work well in practice?

@Elchi3
Copy link
Member

Elchi3 commented Mar 15, 2021

Since all of the allowed ranged versions are pretty far in the past now, I wonder if just treating them an exact version on MDN wouldn't work well in practice?

Can you open a new issue on this proposal and paste in a list of data points affected?

@foolip
Copy link
Collaborator

foolip commented Mar 15, 2021

Sure, I've filed mdn/yari#3238.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
data:api 🐇 Compat data for Web APIs. https://developer.mozilla.org/docs/Web/API
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants