Skip to content

errors: display original symbol name #36042

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

Closed
bcoe wants to merge 7 commits into from
Closed

errors: display original symbol name #36042

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
56d7975
errors: display original symbol name
Nov 8, 2020
f46eb0c
refactor: refactor approch based on research
Nov 27, 2020
a5f883c
Update lib/internal/source_map/source_map.js
Nov 27, 2020
25c45f9
chore: make sure symbols are correct
Nov 27, 2020
8b4a2b1
docs: slight tweaks to comments
Nov 28, 2020
e469554
test: fix up tests; add test for enclosing function
Nov 28, 2020
2624239
test: fix up test output
Nov 28, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
errors: display original symbol name 
If symbol names array has been populated in source map, include
original symbol name in error message.

Fixes #35325
  • Loading branch information
bcoe committed Nov 26, 2020
commit 56d7975df745fec2aed619c716bbd47dbaf5265c
1 change: 1 addition & 0 deletions doc/api/module.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,7 @@ consists of the following keys:
* originalSource: {string}
* originalLine: {number}
* originalColumn: {number}
* name: {string}

[CommonJS]: modules.md
[ES Modules]: esm.md
Expand Down
9 changes: 5 additions & 4 deletions lib/internal/source_map/prepare_stack_trace.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,14 @@ const prepareStackTrace = (globalThis, error, trace) => {
const {
originalLine,
originalColumn,
originalSource
originalSource,
name
Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Nov 8, 2020

Choose a reason for hiding this comment

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

nit: consider adding a trailing comma to make future PR diffs cleaner?

Suggested change
name
name,

Copy link
Copy Markdown
Member

@ljharb ljharb Nov 8, 2020

Choose a reason for hiding this comment

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

I think this is a style that a separate PR should use linting to enforce, and also apply everywhere. (iow i think it’s a great change, but i don’t think node’s followed it consistently thus far)

Copy link
Copy Markdown
Contributor

@aduh95 aduh95 Nov 8, 2020

Choose a reason for hiding this comment

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

Agreed, to be clear I'm +1 landing this with or without the trailing commas.

} = sm.findEntry(t.getLineNumber() - 1, t.getColumnNumber() - 1);
if (originalSource && originalLine !== undefined &&
originalColumn !== undefined) {
if (i === 0) {
firstLine = originalLine + 1;
firstColumn = originalColumn + 1;

// Show error in original source context to help user pinpoint it:
errorSource = getErrorSource(
sm.payload,
Expand All @@ -81,11 +81,12 @@ const prepareStackTrace = (globalThis, error, trace) => {
);
}
// Show both original and transpiled stack trace information:
const prefix = name ? `\n -> at ${name}` : '\n ->';
const originalSourceNoScheme =
StringPrototypeStartsWith(originalSource, 'file://') ?
fileURLToPath(originalSource) : originalSource;
str += `\n -> ${originalSourceNoScheme}:${originalLine + 1}:` +
`${originalColumn + 1}`;
str += `${prefix} (${originalSourceNoScheme}:${originalLine + 1}:` +
Copy link
Copy Markdown
Contributor Author

@bcoe bcoe Nov 8, 2020 •
edited
Loading

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:

Error: an exception
    at branch (/Users/bencoe/oss/node-1/test/fixtures/source-map/typescript-throw.js:20:15)
        -> (/Users/bencoe/oss/node-1/test/fixtures/source-map/typescript-throw.ts:18:11)

Edit: figured while we're calling the feature experimental, it's the time to make these tweaks.

Copy link
Copy Markdown
Member

@ljharb ljharb Nov 28, 2020

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.

Copy link
Copy Markdown
Contributor Author

@bcoe bcoe Nov 28, 2020

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.

Copy link
Copy Markdown
Member

@ljharb ljharb Nov 28, 2020

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 :-/

Copy link
Copy Markdown
Contributor Author

@bcoe bcoe Nov 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

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 :-/

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:

  • 6.1m projects rely source-map-support, to provide better debugging information in stack traces are used.
  • A healthy number of folks are finding --enable-source-maps (my team is using it ourselves because it speeds up our debugging during test).
  • Threads like this indicate that folks are more concerned about transpiled server 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:

at o (/Users/bencoe/oss/node-sourcemap-repro/dist/main.js:1:970)
      -> at functionD (webpack:///index.js:14:9)

☝️ the tabbed in line without an at is fairly distinct.

Copy link
Copy Markdown
Contributor Author

@bcoe bcoe Nov 30, 2020

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.

Copy link
Copy Markdown
Member

@ljharb ljharb Dec 1, 2020

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.

Copy link
Copy Markdown
Member

@ljharb ljharb Dec 1, 2020

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?

`${originalColumn + 1})`;
}
}
} catch (err) {
Expand Down
14 changes: 9 additions & 5 deletions lib/internal/source_map/source_map.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,8 @@ class SourceMap {
generatedColumn: entry[1],
originalSource: entry[2],
originalLine: entry[3],
originalColumn: entry[4]
originalColumn: entry[4],
name: entry[5]
Comment thread
bcoe marked this conversation as resolved.
Outdated
};
}

Expand All @@ -214,6 +215,7 @@ class SourceMap {
let sourceIndex = 0;
let sourceLineNumber = 0;
let sourceColumnNumber = 0;
let nameIndex = 0;

const sources = [];
const originalToCanonicalURLMap = {};
Expand All @@ -229,6 +231,7 @@ class SourceMap {

const stringCharIterator = new StringCharIterator(map.mappings);
let sourceURL = sources[sourceIndex];
let name = map.names?.[nameIndex];

while (true) {
if (stringCharIterator.peek() === ',')
Expand Down Expand Up @@ -256,12 +259,13 @@ class SourceMap {
}
sourceLineNumber += decodeVLQ(stringCharIterator);
sourceColumnNumber += decodeVLQ(stringCharIterator);
if (!isSeparator(stringCharIterator.peek()))
// Unused index into the names list.
decodeVLQ(stringCharIterator);
if (!isSeparator(stringCharIterator.peek())) {
nameIndex += decodeVLQ(stringCharIterator);
name = map.names?.[nameIndex];
}

this.#mappings.push([lineNumber, columnNumber, sourceURL,
sourceLineNumber, sourceColumnNumber]);
sourceLineNumber, sourceColumnNumber, name]);
}
};
}
Expand Down
20 changes: 13 additions & 7 deletions test/parallel/test-source-map-enable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,12 +170,15 @@ function nextdir() {
'--enable-source-maps',
require.resolve('../fixtures/source-map/uglify-throw.js')
]);
assert.ok(
output.stderr.toString().match(/->.*uglify-throw-original\.js:5:9/)
assert.match(
output.stderr.toString(),
/->.*uglify-throw-original\.js:5:9/
);
assert.ok(
output.stderr.toString().match(/->.*uglify-throw-original\.js:9:3/)
assert.match(
output.stderr.toString(),
/->.*uglify-throw-original\.js:9:3/
);
assert.match(output.stderr.toString(), /at Hello/);
}

// Applies source-maps generated by tsc to stack trace.
Expand Down Expand Up @@ -276,11 +279,14 @@ function nextdir() {
require.resolve('../fixtures/source-map/webpack.js')
]);
// Error in original context of source content:
assert.ok(
output.stderr.toString().match(/throw new Error\('oh no!'\)\r?\n.*\^/)
assert.match(
output.stderr.toString(),
/throw new Error\('oh no!'\)\r?\n.*\^/
);
// Rewritten stack trace:
assert.ok(output.stderr.toString().includes('webpack:///webpack.js:14:9'));
assert.match(output.stderr.toString(), /webpack:\/\/\/webpack\.js:14:9/);
assert.match(output.stderr.toString(), /at functionD/);
assert.match(output.stderr.toString(), /at functionC/);
}

// Stores and applies source map associated with file that throws while
Expand Down