-
Notifications
You must be signed in to change notification settings - Fork 30.2k
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
errors: display original symbol name #36042
Closed
Closed
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
56d7975
errors: display original symbol name
bcoe f46eb0c
refactor: refactor approch based on research
bcoe a5f883c
Update lib/internal/source_map/source_map.js
bcoe 25c45f9
chore: make sure symbols are correct
bcoe 8b4a2b1
docs: slight tweaks to comments
bcoe e469554
test: fix up tests; add test for enclosing function
bcoe 2624239
test: fix up test output
bcoe File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
var functionA=function(){functionB()};function functionB(){functionC()}var functionC=function(){functionD()},functionD=function(){if(0<Math.random())throw Error("an error!");},thrower=functionA;try{functionA()}catch(a){throw a;}; | ||
//# sourceMappingURL=enclosing-call-site.js.map | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
const functionA = () => { | ||
functionB() | ||
} | ||
|
||
function functionB() { | ||
functionC() | ||
} | ||
|
||
const functionC = () => { | ||
functionD() | ||
} | ||
|
||
const functionD = () => { | ||
(function functionE () { | ||
if (Math.random() > 0) { | ||
throw new Error('an error!') | ||
} | ||
})() | ||
} | ||
|
||
const thrower = functionA | ||
|
||
try { | ||
thrower() | ||
} catch (err) { | ||
throw err | ||
} |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
// Flags: --enable-source-maps | ||
|
||
'use strict'; | ||
require('../common'); | ||
require('../fixtures/source-map/enclosing-call-site-min.js'); |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
*enclosing-call-site.js:16 | ||
throw new Error('an error!') | ||
^ | ||
|
||
Error: an error! | ||
at functionD (*enclosing-call-site-min.js:1:156) | ||
-> (*enclosing-call-site.js:16:17) | ||
at functionC (*enclosing-call-site-min.js:1:97) | ||
-> (*enclosing-call-site.js:10:3) | ||
at functionB (*enclosing-call-site-min.js:1:60) | ||
-> (*enclosing-call-site.js:6:3) | ||
at functionA (*enclosing-call-site-min.js:1:26) | ||
-> (*enclosing-call-site.js:2:3) | ||
at Object.<anonymous> (*enclosing-call-site-min.js:1:199) | ||
-> (*enclosing-call-site.js:24:3) | ||
at Module._compile (node:internal/modules/cjs/loader:*) | ||
at Object.Module._extensions..js (node:internal/modules/cjs/loader:*) | ||
at Module.load (node:internal/modules/cjs/loader:*) | ||
at Function.Module._load (node:internal/modules/cjs/loader:*) | ||
at Module.require (node:internal/modules/cjs/loader:*) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
Worth noting that this puts a
(
,)
around the file paths in supplemental stack traces now. I felt that this looked better when we have an alternate symbol name, and it seemed more consistent to always wrap in parenthesis:Edit: figured while we're calling the feature experimental, it's the time to make these tweaks.
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.
as a heads up, this might make it incompatible with the tc39 stacks proposal, if it ever lands - it’ll only specify the union of what browsers do.
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.
@ljharb what would you suggest we do to provide appropriate context about the transpiled call site and original call site, in the spirit of the spirit of the tc39 stacks proposal?
I'm hoping to make the case for removing the experimental language from
--enable-source-maps
soon -- so now would be a good time to make significant changes to output if needed.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.
The challenge is specifying what browsers all already do, without permitting anything beyond that.
What do browsers do here on source mapped exception stacks? If the answer is “nothing special”, then that’s probably what node should do too :-/
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.
TypeScript and other transpiled flavors or JavaScript have exploded in popularity over the past several years, I'd argue that stack traces just haven't caught up with this need:
source-map-support
, to provide better debugging information in stack traces are used.--enable-source-maps
(my team is using it ourselves because it speeds up our debugging during test).transpiledserver code, when it comes to using contextual information about the stack.I would have originally gone with the approach
source-map-support
, of simply replacing the transpiled Call Site with the original Call Site. I believe @hashseed had the idea of displaying both the source and original Call Site, and I believe there's value in this, for a variety of reasons: when looking at an error, it can be good to see how the source was transpiled; source maps, despite best efforts, are sometimes off.It would be nice if the stacks proposal gave us enough flexibility that we could insert this extra contextual information into the stack trace, without breaking upstream parser.
Couldn't we just consider this to be one line of the stack trace:
☝️ the tabbed in line without an
at
is fairly distinct.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.
@ljharb let's move this conversation to the TC39 proposal; this PR is just patching a bug, not introducing new behavior to Node.js.
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.
per our offline discussion, it might be simpler for now to leave
.stack
as having only one of the results (source, or transpiled), and maybe providing a separate API (Error.getOriginalStack()
,Error.getUnmappedStack()
) to access the other one? That would ensure no conflicts with the stacks proposal.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.
Did this land as-is, with the parentheticals discussed here?