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.

Sign up for GitHub

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

Jump to bottom

errors: remove eager stack generation for node errors #39182

Merged
merged 1 commit into from
Jul 8, 2021
Merged

errors: remove eager stack generation for node errors #39182

merged 1 commit into from
Jul 8, 2021

Conversation

devsnek
Copy link
Member

@devsnek devsnek commented Jun 28, 2021 •
edited
Loading

Moves logic from addCodeToName to prepareStackTrace, which saves some upfront perf and also allows node errors to be overridden by overrideStackTrace.

cc @guybedford

@github-actions github-actions bot added errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem. needs-ci PRs that need a full CI run. labels Jun 28, 2021
guybedford
guybedford approved these changes Jul 2, 2021
View reviewed changes
Copy link
Contributor

@guybedford guybedford 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 working on this, seems good to me. If anything the extended outputs on the cases that changed seem like an improvement as well.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38831/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38833/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38834/

@devsnek devsnek requested a review from BridgeAR July 3, 2021 04:08
@devsnek
Copy link
Member Author

devsnek commented Jul 3, 2021

thanks for fixing this up guy. added ruben since this touches the repl tests.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/38835/

@devsnek devsnek removed the needs-ci PRs that need a full CI run. label Jul 3, 2021
@guybedford
Copy link
Contributor

Strictly speaking this PR can land now. Did you still want to wait on a review from @BridgeAR further here Gus?

@devsnek
Copy link
Member Author

devsnek commented Jul 5, 2021

🤷‍♂️ maybe give it a couple more days

@devsnek
errors: remove eager stack generation for node errors
1782170 
PR-URL: nodejs#39182
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@devsnek devsnek force-pushed the better-error-stuff branch from 3943432 to 1782170 Compare July 8, 2021 01:53
@devsnek devsnek merged commit 1782170 into nodejs:master Jul 8, 2021
@devsnek devsnek deleted the better-error-stuff branch July 8, 2021 01:53
targos pushed a commit that referenced this pull request Jul 11, 2021
@devsnek @targos
errors: remove eager stack generation for node errors
33cad27 
PR-URL: #39182
Reviewed-By: Guy Bedford <guybedford@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 13, 2021
Merged
@cspotcode cspotcode mentioned this pull request Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guybedford guybedford guybedford approved these changes

@jasnell jasnell jasnell approved these changes

@BridgeAR BridgeAR Awaiting requested review from BridgeAR

Assignees
No one assigned
Labels
errors Issues and PRs related to JavaScript errors originated in Node.js core. http2 Issues or PRs related to the http2 subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@devsnek @nodejs-github-bot @guybedford @jasnell @targos