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.

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

--enable-source-maps adds error context before stack string #43186

Closed
victorandree opened this issue May 23, 2022 · 3 comments
Closed

--enable-source-maps adds error context before stack string #43186

victorandree opened this issue May 23, 2022 · 3 comments
Labels
source maps Issues and PRs related to source map support.

Comments

@victorandree
Copy link

Version

v18.2.0

Platform

Darwin mbp.local 21.5.0 Darwin Kernel Version 21.5.0: Tue Apr 26 21:08:29 PDT 2022; root:xnu-8020.121.3~4/RELEASE_ARM64_T8101 arm64

Subsystem

No response

What steps will reproduce the bug?

When accessing Error.stack without source maps, only the error string and error frames are produced. However, when using --enable-source-maps, the context of the original error is prefixed to this output. I don't think this expected behavior (see below for why).

To reproduce, compile the following TypeScript program with tsc --sourceMap true:

function main() {
  try {
    throw new Error('Message');
  } catch (err: any) {
    console.error(err.stack);
  }
}

main();

How often does it reproduce? Is there a required condition?

You need to use the --enable-source-maps command line flag when running Node.

What is the expected behavior?

When turning on source maps, I only expect the symbols and filenames to be rewritten:

$ node --enable-source-maps build/app.js
Error: Message
    at main (/app.ts:3:11)
    at Object.<anonymous> (/app.ts:9:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

I don't expect that turning on source maps would do anything but rewrite the symbols and filenames using source maps. While this may be helpful, it significantly changes the output of Error.stack, in a way that is not compatible with the TC39 proposal for error stacks, nor with standard V8 output that tools rely on.

Including an "error context" is not part of the Stage 1 Error Stacks proposal, where the specification for the GetStackString operation specifies how to construct the "stack string":

3 GetStackString ( error )

When the abstract operation GetStackString is called with argument error, the following steps are performed:
[...]
6. Let stackString be the concatenation of errorString, the code unit 0x000A (LINE FEED), and frameString.

This can also break systems that rely on errors being reported exactly as returned by V8. For example, Google Cloud Error Reporting requires that you log errors exactly as returned by V8 in order for them to get picked up. See also:

What do you see instead?

The output includes an "error context" prefixed to the stack, that looks like this:

$ node --enable-source-maps build/app.js
/app.ts:3
    throw new Error('Message');
          ^

Error: Message
    at main (/app.ts:3:11)
    at Object.<anonymous> (/app.ts:9:1)
    at Module._compile (node:internal/modules/cjs/loader:1105:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
    at Module.load (node:internal/modules/cjs/loader:981:32)
    at Module._load (node:internal/modules/cjs/loader:827:12)
    at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
    at node:internal/main/run_main_module:17:47

Additional information

#37252 would change how --enable-source-maps output is formatted, with the purpose of aligning with the Error Stacks proposal. However, this PR retains the printing of an error context above the "actual" stack (formatted according to the proposal). It therefore seems to be a goal to follow the spec—but I think including context violates the spec.

Printing the original exception context was introduced in #33491. This PR references that before this change, the transpiled source would be displayed. I'm not sure exactly what the format was back then, or if any context was printed.

@benjamingr
Copy link
Member

To be fair the error stack proposal is very early stage and may change and I would caution aligning to it and Node is the platform so expecting Node to align its core features to google cloud error reporting seems a bit backwards.

That said: I can see a consistency argument here and I'm wondering what users would find better.

cc @bcoe

@bcoe
Copy link
Contributor

bcoe commented Jun 2, 2022

The reason "error context" is added, was an effort to align behavior close with Node.js' treatment of errors when --enable-source-maps is not enabled:

const foo = () => {
  throw Error('beep boop');
}
foo()
$ node foo.js 
/Users/benjamincoe/foo.js:2
  throw Error('beep boop');
  ^

Error: beep boop
    at foo (/Users/benjamincoe/foo.js:2:9)
    at Object.<anonymous> (/Users/benjamincoe/foo.js:4:1)
    at Module._compile (internal/modules/cjs/loader.js:1085:14)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:1114:10)
    at Module.load (internal/modules/cjs/loader.js:950:32)
    at Function.Module._load (internal/modules/cjs/loader.js:790:12)
    at Function.executeUserEntryPoint [as runMain] (internal/modules/run_main.js:76:12)
    at internal/main/run_main_module.js:17:47
Benjamins-MacBook-Pro:~ benjamincoe$ 

The decision to include the additional context has been a bit of a nuisance, one issue being it hurts performance.

I'd be supportive of dropping the "error context" aligning more closely with the specification. However, I think this would need to be considered a breaking change.

@legendecas
Copy link
Member

Just for context: #41541 (comment). And I'm working on this. :)

danielleadams pushed a commit that referenced this issue Aug 16, 2022
The source context is not prepended to the value of the `stack` property
when the source map is not enabled. Rather than prepending the error
source context to the value of the `stack` property unconditionally,
this patch aligns the behavior and only prints the source context when
the error is not handled by userland (e.g. fatal errors).

Also, this patch fixes that when source-map support is enabled, the
error source context is not pointing to where the error was thrown.

PR-URL: #43875
Fixes: #43186
Fixes: #41541
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
ruyadorno pushed a commit that referenced this issue Aug 23, 2022
The source context is not prepended to the value of the `stack` property
when the source map is not enabled. Rather than prepending the error
source context to the value of the `stack` property unconditionally,
this patch aligns the behavior and only prints the source context when
the error is not handled by userland (e.g. fatal errors).

Also, this patch fixes that when source-map support is enabled, the
error source context is not pointing to where the error was thrown.

PR-URL: #43875
Fixes: #43186
Fixes: #41541
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Fyko pushed a commit to Fyko/node that referenced this issue Sep 15, 2022
The source context is not prepended to the value of the `stack` property
when the source map is not enabled. Rather than prepending the error
source context to the value of the `stack` property unconditionally,
this patch aligns the behavior and only prints the source context when
the error is not handled by userland (e.g. fatal errors).

Also, this patch fixes that when source-map support is enabled, the
error source context is not pointing to where the error was thrown.

PR-URL: nodejs#43875
Fixes: nodejs#43186
Fixes: nodejs#41541
Reviewed-By: Ben Coe <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
source maps Issues and PRs related to source map support.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants