GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — doc: clarify `uncaughtException` `origin` for ESM by aduh95 · Pull Request #41339 · nodejs/node

Conversation

@aduh95

@aduh95 aduh95 commented Dec 27, 2021

Copy link
Copy Markdown
Contributor

Fixes: #41328

/cc @nodejs/modules

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bricss reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aduh95
doc: clarify uncaughtException origin for ESM
df5db76
Fixes: nodejs#41328
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to Node.js documentation. process Issues and PRs related to the process subsystem. labels Dec 27, 2021
GeoffreyBooth
GeoffreyBooth approved these changes Dec 27, 2021
View reviewed changes
bmeck
bmeck reviewed Dec 27, 2021
View reviewed changes
Comment thread doc/api/process.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

bmeck
bmeck suggested changes Dec 27, 2021
View reviewed changes

@bmeck bmeck left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

should workshop wording to make it clear that it isn't causes necessarily by ESM entrypoints but just having things wrapped in a Promise async context in general, see a purely CJS duplication:

// node --unhandled-rejections=strict entry.cjs
new Promise(() => require('./throws.cjs'));
// throws.cjs
process.on('uncaughtExceptionMonitor', (...args) => {
  console.log(args);
})
throw new Error('thrown');

might mean just more clearly defining what an uncaughtException is since the terms are being used vaguely.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

ljharb reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@aduh95

This comment has been minimized.

Sign in to view
@aduh95

aduh95 commented Dec 27, 2021

Copy link
Copy Markdown
Contributor Author

Hum it looks like you're correct, using the entry point is a red herring:

mkdir repro
cd repro
echo 'process.on("uncaughtException", (_, origin) => console.log(origin));nonexistentFunc()' > test.cjs
node test.cjs # uncaughtException
node --experimental-specifier-resolution=node --unhandled-rejections=none test.cjs # unhandledRejection
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

aduh95 added 4 commits December 27, 2021 18:20
@aduh95
fixup! doc: clarify uncaughtException origin for ESM
c3b2692
@aduh95
fixup! doc: clarify uncaughtException origin for ESM
61eb69a
@aduh95
fixup! doc: clarify uncaughtException origin for ESM
ffe2656
@aduh95
fixup! doc: clarify uncaughtException origin for ESM
c8d2454
bmeck
bmeck reviewed Dec 27, 2021
View reviewed changes
Comment thread doc/api/cli.md Resolved

Uh oh!

There was an error while loading. Please reload this page.

bmeck
bmeck reviewed Dec 27, 2021
View reviewed changes
Comment thread doc/api/process.md Outdated
[`--unhandled-rejections`][] flag set to `strict` or `throw` and
an unhandled rejection.
an unhandled rejection, or when a rejection happens during the ES module
static loading phase.

@bmeck bmeck Dec 27, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

can we just simplify this to when in a Promise based async context? Since purely with CJS and having no ESM this also happens in some cases.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 27, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you have a purely CJS example when this happen when --unhandled-rejections flag is NOT set to strict or throw?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@bmeck bmeck Dec 27, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@aduh95 I do not for that case, but it does happen outside of ES module static loading phase.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

it does happen outside of ES module static loading phase

Any chance we can list those edge cases as well? I don't think when in a Promise based async context does a great job at explaining when this will come up – my take away from #41328 is that it's not obvious to everyone that ESM loading is Promise based, so imho it's worth explicitly list it here rather than expect readers to know what is a Promise based context.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@GeoffreyBooth GeoffreyBooth Dec 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Do you have a purely CJS example when this happen when --unhandled-rejections flag is NOT set to strict or throw?

Didn’t the default for --unhandled-rejections change in 17? So it doesn’t need to be set anymore to get the error.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

It changed in v15.0.0. But that's beside the point I think, this section of the docs is currently wrong, and if this PR doesn't do a good enough job to document when users should expect unhandledRejection rather than uncaughtException, I'm open to suggestions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@bmeck bmeck Dec 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

@aduh95 roughly anything that creates a Promise job causes that, so IDK where to start. If an uncaughtException would occur inside of an async function / inside of a Promise constructor/handler callbacks / inside ESM are all the real times that this happens. So uncaughtException isn't really about the source file / throwing at the top level, but it is about how it was invoked, always. Listing async functions and Promise methods might be enough?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@ljharb ljharb Dec 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

async functions, promise then/catch/finally, new Promise executor functions, and soon, Array.fromAsync

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@aduh95 aduh95 Dec 28, 2021

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

anything that creates a Promise job causes that, so IDK where to start

@bmeck I think that is true when --unhandled-rejections is set to strict or throw (which is default), and we're in agreement on this part. I'll try to rephrase the sentence so this part is clearer.

What is more tricky is that a rejection in the ES loader will always trigger an 'uncaughtException' event with origin 'unhandledRejection', even if --unhandled-rejections is set to none (or warn or warn-with-error-code). At least to me, this part is surprising, and in contradiction with what the docs are currently saying, so I think it's worth documenting it. I believe that the ES module loader is the only piece of node that behaves like this, but if you know of another example, I can include it.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

vladshcherbin reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

@bmeck bmeck Dec 28, 2021

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I can't think of another off the top of my head. I think we've done enough workshopping though that whatever is clarified seems good at this point.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@aduh95
fixup! doc: clarify uncaughtException origin for ESM
b589d69
bmeck
bmeck approved these changes Dec 28, 2021
View reviewed changes
@aduh95 aduh95 added author ready PRs with CI started, the required approvals, and no outstanding review comments. commit-queue PRs queued for automated landing through the Commit Queue. labels Jan 9, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Jan 9, 2022
@nodejs-github-bot
nodejs-github-bot merged commit 7edb50d into nodejs:master Jan 9, 2022
@nodejs-github-bot

nodejs-github-bot commented Jan 9, 2022

Copy link
Copy Markdown
Collaborator

Landed in 7edb50d

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vladshcherbin

vladshcherbin commented Jan 9, 2022

Copy link
Copy Markdown

Thanks for addition!

As a non-native speaker I'll never understand what this means in the docs.
I also do believe even for a native speaker it's hard to understand the use-case.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@aduh95
aduh95 deleted the esm-unhandleRejection branch January 9, 2022 12:20
@aduh95

aduh95 commented Jan 9, 2022

Copy link
Copy Markdown
Contributor Author

@vladshcherbin would you like to send a PR with an improved wording? Or if you have a specific suggestion I can send the PR myself if you prefer.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos pushed a commit that referenced this pull request Jan 14, 2022
@aduh95 @targos
doc: clarify uncaughtException origin for ESM
edc6a7a
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
@targos targos mentioned this pull request Jan 16, 2022
v17.4.0 release proposal #41557
Merged
danielleadams pushed a commit that referenced this pull request Feb 1, 2022
@aduh95 @danielleadams
doc: clarify uncaughtException origin for ESM
52a988b
Fixes: #41328

PR-URL: #41339
Reviewed-By: Geoffrey Booth <[email protected]>
Reviewed-By: Bradley Farias <[email protected]>
@danielleadams danielleadams mentioned this pull request Feb 1, 2022
v16.14.0 proposal #41804
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ljharb ljharb ljharb left review comments
@GeoffreyBooth GeoffreyBooth GeoffreyBooth approved these changes
+1 more reviewer
@bmeck bmeck bmeck approved these changes
Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. doc Issues and PRs related to Node.js documentation. process Issues and PRs related to the process subsystem.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

uncaughtException origin is always unhandledRejection when package.json type is module

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@aduh95 @nodejs-github-bot @vladshcherbin @ljharb @bmeck @GeoffreyBooth
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.