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

feat: add "unhandledrejection" event support #12994

Merged
merged 23 commits into from
Jul 4, 2022

Conversation

bartlomieju
Copy link
Member

@bartlomieju bartlomieju commented Dec 5, 2021

This commit adds support for "unhandledrejection" event.

This event will trigger event listeners registered using:

  • "globalThis.addEventListener("unhandledrejection")
  • "globalThis.onunhandledrejection"

This is done by registering a default handler using
"Deno.core.setPromiseRejectCallback" that allows to
handle rejected promises in JavaScript instead of Rust.

This commit will make it possible to polyfill
"process.on("unhandledRejection")" in the Node compat
layer.

Closes #7013

Copy link
Contributor

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, Bartek! The test failures are probably (hopefully) a small matter of updating test expectations.

Comment on lines +1270 to +1271
// TODO(lucacasonato): remove when this interface is spec aligned
[SymbolToStringTag] = "PromiseRejectionEvent";
Copy link
Contributor

Choose a reason for hiding this comment

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

For my understanding: what is the misalignment here? cc @lucacasonato

Copy link
Member

Choose a reason for hiding this comment

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

The entire eventing system has not been migrated to use our webidl helper classes yet. It is on my backburner.

runtime/js/99_main.js Outdated Show resolved Hide resolved
@bartlomieju
Copy link
Member Author

Thanks for doing this, Bartek! The test failures are probably (hopefully) a small matter of updating test expectations.

I think there's quite more to do before this PR will be ready to merge. Just wanted to get ball rolling for now.

@bartlomieju
Copy link
Member Author

Discussed with @bnoordhuis offline, we're punting this PR for now as it's not clear how this functionality would integrate with Node compatibility layer. Ben will first implement process.on("unhandledRejection", ...) in deno_std/node.

@bartlomieju bartlomieju added this to the 1.18.0 milestone Dec 7, 2021
@lucsoft
Copy link

lucsoft commented Dec 7, 2021

Does this PR not implement document.addEventListener("unhandledrejection")?

@bartlomieju
Copy link
Member Author

Does this PR not implement document.addEventListener("unhandledrejection")?

It does implement it.

@Skillz4Killz
Copy link

As a reminder, it is incredibly useful to some people to be able to make sure this can be used to prevent a process from crashing in production.

@bartlomieju bartlomieju removed this from the 1.18.0 milestone Jan 16, 2022
@hashemisa
Copy link

What version should we wait for this possibility?

@stale
Copy link

stale bot commented Mar 29, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Mar 29, 2022
@stale stale bot closed this Apr 6, 2022
@bartlomieju bartlomieju reopened this Apr 7, 2022
@stale stale bot removed the stale label Apr 7, 2022
@jiawei397
Copy link

Do you have any plans to merge this submission?

@bartlomieju
Copy link
Member Author

Do you have any plans to merge this submission?

Yes, but it hasn't been the highest priority.

@jiawei397
Copy link

Do you have any plans to merge this submission?

Yes, but it hasn't been the highest priority.

OK, I will continue to pay attention to this. Thanks for your reply.

@bartlomieju bartlomieju changed the title [WIP] feat: add window.onunhandledrejection feat: add window.onunhandledrejection Jun 28, 2022
@bartlomieju bartlomieju changed the title feat: add window.onunhandledrejection feat: add "unhandledrejection" event support Jun 28, 2022
Copy link
Contributor

@kitsonk kitsonk left a comment

Choose a reason for hiding this comment

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

LGTM

@bartlomieju
Copy link
Member Author

bartlomieju commented Jul 4, 2022

Note to self: add test for when promise rejects with "undefined" to check that "reason" on the event is actually "undefined" instead of "null".

Done

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

LGTM!

@bartlomieju bartlomieju merged commit f7af0b0 into denoland:main Jul 4, 2022
@bartlomieju bartlomieju deleted the unhandledrejections branch July 4, 2022 19:15
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jul 4, 2022
bartlomieju added a commit that referenced this pull request Jul 4, 2022
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jul 5, 2022
This commit adds support for "unhandledrejection" event.

This event will trigger event listeners registered using:

"globalThis.addEventListener("unhandledrejection")
"globalThis.onunhandledrejection"
This is done by registering a default handler using
"Deno.core.setPromiseRejectCallback" that allows to
handle rejected promises in JavaScript instead of Rust.

This commit will make it possible to polyfill
"process.on("unhandledRejection")" in the Node compat
layer.

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
bartlomieju added a commit that referenced this pull request Jul 14, 2022
Relanding #12994 

This commit adds support for "unhandledrejection" event.

This event will trigger event listeners registered using:

"globalThis.addEventListener("unhandledrejection")
"globalThis.onunhandledrejection"
This is done by registering a default handler using
"Deno.core.setPromiseRejectCallback" that allows to
handle rejected promises in JavaScript instead of Rust.

This commit will make it possible to polyfill
"process.on("unhandledRejection")" in the Node compat
layer.

Co-authored-by: Colin Ihrig <cjihrig@gmail.com>
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jul 14, 2022
bartlomieju added a commit that referenced this pull request Jul 14, 2022
bartlomieju added a commit to bartlomieju/deno that referenced this pull request Jul 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

onunhandledrejection for globalThis?
9 participants