`--enable-source-maps` adds error context before stack string

[victorandree]

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:

• https://cloud.google.com/error-reporting/docs/formatting-error-messages#formatting_requirements
• https://cloud.google.com/error-reporting/reference/rest/v1beta1/projects.events/report#ReportedErrorEvent

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]

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]

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]

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