Skip to content

doc: improve example for Error.captureStackTrace() - #46886

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
brodo:capture-stack-trace-example-improvement
Mar 26, 2023
Merged

doc: improve example for Error.captureStackTrace()#46886
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
brodo:capture-stack-trace-example-improvement

Conversation

@brodo

Copy link
Copy Markdown
Contributor

Change the MyError example so that instances of MyError are instanceof Error and also native errors when checked with util.types.isNativeError().

@nodejs-github-botnodejs-github-bot added doc Issues and PRs related to the documentations. errors Issues and PRs related to JavaScript errors originated in Node.js core. labels Feb 28, 2023

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I see why you suggest this. I am not certain if the example becomes more difficult to read though, especially as it has new implications as outlined in my comment.

Comment threaddoc/api/errors.md Outdated
@brodo

brodo commented Feb 28, 2023

Copy link
Copy Markdown
ContributorAuthor

Should we maybe add a suggestion to use class MyError extends Error {}? Instances of this class will be instanceof Error and native errors. They will also not show the MyError constructor in the stack trace.

@brodo

brodo commented Feb 28, 2023

Copy link
Copy Markdown
ContributorAuthor

I see why you suggest this. I am not certain if the example becomes more difficult to read though, especially as it has new implications as outlined in my comment.

I understand the readability concern. Essentially it's a trade-off between having a 'copyable' example and a readable one. It would be interesting to find out if there are packages on npm that contain this error implementation...

@BridgeARBridgeAR left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am hesitant to change the documentation like that as we mix different APIs and errors should ideally not be created like that in the first place.

What about changing the example to show how to hide multiple call sites? E.g.,

functiona(){b();}functionb(){c();}functionc(){// Create an error without stack trace to avoid calculating the stack trace twice.const{ stackTraceLimit }=Error;Error.stackTraceLimit=0;consterror=newError();Error.stackTraceLimit=stackTraceLimit;// Capture the stack trace above function bError.captureStackTrace(error,b);// Neither function c, nor b is included in the stack tracethrowerror;}

That way we create a "real" error and only change it's stack trace accordingly.

Comment threaddoc/api/errors.md Outdated
Comment threaddoc/api/errors.md Outdated
@brodo

brodo commented Mar 1, 2023

Copy link
Copy Markdown
ContributorAuthor
functiona(){b();}functionb(){c();}functionc(){// Create an error without stack trace to avoid calculating the stack trace twice.const{ stackTraceLimit }=Error;Error.stackTraceLimit=0;consterror=newError();Error.stackTraceLimit=stackTraceLimit;// Capture the stack trace above function bError.captureStackTrace(error,b);// Neither function c, nor b is included in the stack tracethrownewError();}

I like this example a lot. It illustrates the usage of the function much better. But shouldn't the last line be throw error;?

@brodo

Copy link
Copy Markdown
ContributorAuthor

Is there anything else I need to do to so this can get merged?

@debadree25

debadree25 commented Mar 16, 2023

Copy link
Copy Markdown
Contributor

Could you rebase and update the first commit message, it seems the description exceeds the size limit of 72

@debadree25debadree25 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The example looks good, would also wait for @BridgeAR 's review to see if the changes address all his concerns.

@debadree25debadree25 added the commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. label Mar 16, 2023
Change the `MyError` example so that instances of `MyError`are
`instanceof Error` and also native errors when checked with
`util.types.isNativeError()`.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
@brodo
brodoforce-pushed the capture-stack-trace-example-improvement branch from c4b05b3 to 7d8b38fCompareMarch 18, 2023 11:56
@brodo

Copy link
Copy Markdown
ContributorAuthor

Could you rebase and update the first commit message, it seems the description exceeds the size limit of 72

I've squashed all the commits and added line breaks.

@lpincalpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 26, 2023
@nodejs-github-botnodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Mar 26, 2023
@nodejs-github-bot
nodejs-github-bot merged commit 38e6ac7 into nodejs:mainMar 26, 2023
@nodejs-github-bot

Copy link
Copy Markdown
Collaborator

Landed in 38e6ac7

RafaelGSS pushed a commit that referenced this pull request Apr 5, 2023
Change the `MyError` example so that instances of `MyError`are
`instanceof Error` and also native errors when checked with
`util.types.isNativeError()`.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #46886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@RafaelGSSRafaelGSS mentioned this pull request Apr 6, 2023
RafaelGSS pushed a commit that referenced this pull request Apr 6, 2023
Change the `MyError` example so that instances of `MyError`are
`instanceof Error` and also native errors when checked with
`util.types.isNativeError()`.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #46886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 7, 2023
Change the `MyError` example so that instances of `MyError`are
`instanceof Error` and also native errors when checked with
`util.types.isNativeError()`.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #46886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
danielleadams pushed a commit that referenced this pull request Jul 6, 2023
Change the `MyError` example so that instances of `MyError`are
`instanceof Error` and also native errors when checked with
`util.types.isNativeError()`.
Co-authored-by: Ruben Bridgewater <ruben@bridgewater.de>
PR-URL: #46886
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

commit-queue-squashAdd this label to instruct the Commit Queue to squash all the PR commits into the first one.docIssues and PRs related to the documentations.errorsIssues and PRs related to JavaScript errors originated in Node.js core.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@brodo@debadree25@nodejs-github-bot@jasnell@lpinca@BridgeAR