Error.prepareStackTrace not called when --enable-source-maps

[dougwilson]

• Version: v12.12.0
• Platform: Windows 10 64-bit
• Subsystem: error

It seems that when --enable-source-maps is enabled, the way errors work is altered. For example, a global Error.prepareStackTrace function is no longer called as part of the process.

I know --enable-source-maps is experimental based on the docs, but cannot find any specific information on, for example, if this is an expected change to runtime behavior or not.

Just glancing around at the source code, I believe that internal/errors.js is what is calling the Error.prepareStackTrace when defined, but the command line flag replaces the stack trace implementation to one that does not call Error.prepareStackTrace any longer.

[devsnek]

this isn't exactly a bug, more like a known limitation.

[dougwilson]

this isn't exactly a bug, more like a known limitation.

Well, like I said,

but cannot find any specific information on, for example, if this is an expected change to runtime behavior or not.

If it's a known limitation, is it documented somewhere I'm missing?

[targos]

I've hit this while trying --enable-source-maps on a TypeScript project. One of my dependencies uses the depd module and it breaks on load.

[joyeecheung]

cc @bcoe

The question is even if we call Error.prepareStackTrace when source map is enabled, what should we pass to it? Mapped CallSites with methods overriden with source map support (how? source map does not include all the information that CallSites provide, or that might not make sense for some of them), or just pass the raw CallSites (but then how do we map them again from whatever string that users return?)?

[joyeecheung]

I think if we can't figure out how to make them work together right now we should fix the docs and mention that they do not work together at the moment (that was my review request in #29564 (comment))

[devsnek]

this seems like a nice opportunity to guage how much the ecosystem can be weened off of prepareStackTrace, although it's regrettable we don't have something to replace it with.

[dougwilson]

I'm not sure if someone has a method to look through the npm ecosystem to see the usage. This report came in because, for example, Express will not even start with the command line enabled. Even if it started, it's hard to know what uses it at runtime. There are modules I'm aware of that their entire existence depends on this API like https://www.npmjs.com/package/callsites (160 dependents), https://www.npmjs.com/package/callsite (394 dependents), etc. and also modules that use it for some of their functionality like https://www.npmjs.com/package/resolve (4,385 dependents).

[richardlau]

I'm not sure if someone has a method to look through the npm ecosystem to see the usage.

I think that's what Gzemnid is for.

[bcoe]

One option would be that we move the logic outside of prepareStackTrace to a method that gets called immediately following prepareStackTrace, my concern is that you could end up in a world whee a module like source-map-support steps on the toes of --enable-source-maps, and you end up with a fairly garbled stack trace.

Perhaps we should just update the documentation to indicate that it does not work in conjunction with prepareStackTrace ... this is an experimental feature, so it's my hope we gradually out these kinks as we test.

[bcoe]

@dougwilson @joyeecheung @devsnek before I update docs, should we consider adding this logic to our source-map support?

  // Polyfill of V8's Error.prepareStackTrace API.
  // https://crbug.com/v8/7848
  // `globalThis` is the global that contains the constructor which
  // created `error`.
  if (typeof globalThis.Error.prepareStackTrace === 'function') {
    return globalThis.Error.prepareStackTrace(error, trace);
  }

We could even consider not returning, and attempting to rework the stack trace after calling the globalThis.Error.prepareStackTrace.

@dougwilson what are your thoughts:

1. does it make sense to try to annotate the response from the user-land prepareStackTrace.
2. would you make a strong case for this functionality ... or should we try to deprecate it, like @devsnek is advocating?

[devsnek]

I think if we want to support prepareStackTrace with source maps, we should create some fake call sites and pass them.

[dougwilson]

So as for what to do, from what I can tell about a lot of this modules on npm that use this, they typically want to access the stack information (vs format it).

If I were to advocate for something specifically, I would probably say in the following order:

1. If userland defines it's own prepareStackTrace, then the command line switch does effectively nothing, as the userland one will be invoked. This would position it as swapping the default prepareStackTrace vs making it unchangable.
2. In the best world, it would be like @devsnek says, where what the command line switch does is not format the stacks, but instead alters them. Perhaps that would mean the original stack trace formatter would then stay intact.

I'm not 100% on either of those, as I can see merits in either one. It would be cool to hear from other folks who use this as well in userland; I'm just the unfortunate first victim due to the popularity of Express

[bcoe]

I don't love simply adding the new call sites, because I think it would become harder to differentiate between the original source and transpiled source, I feel there's value in the formatting of:

orginal call site
  -> call site with with source map applied.

Perhaps the best of both worlds would be:

  // Polyfill of V8's Error.prepareStackTrace API.
  // https://crbug.com/v8/7848
  // `globalThis` is the global that contains the constructor which
  // created `error`.
  if (typeof globalThis.Error.prepareStackTrace === 'function') {
     addFakeCallSites(trace)
     return globalThis.Error.prepareStackTrace(error, trace);
  }
  printPrettyStackTrace();

[joyeecheung]

Perhaps the best of both worlds would be

It is possible that the user actually wants the original callsites e.g. if they want to parse th original stack trace and read the files, IIRC that's what our assert module does (did?) to annotate the diff of objects. Then if the original file does not exist on the machine that runs the code this polyfill still breaks things.

If we rush into adding some support that do not fit the actual use cases well it might result in another maintainance burden like what Error.prepareStackTrace already is. So I think before we figure things out we should still just update the docs saying that they do not work together at the moment, at least that would be less surprising for users. It's an experimental feature after all so I think it's fine to say that it does not work perfectly with everything while it's experimental. It's also perfectly fine to come back and update the docs saying "yay they work perfectly fine with each other now and here is what you need to do to make them work".

[addaleax]

Can we mayble simply add another property to the call site object, e.g. callSite.sourceMapped (which would look like the original callSite objects with the relevant information exchanged)?

Either way, I think we’ll have to consider Error.prepareStackTrace part of our API, and not something that the ecosystem should (have to) be moved away from, at least not short-term.

[devsnek]

I am very strongly against adding any API surface to any part of prepareStackTrace.

[addaleax]

@devsnek I’m not a fan of that idea, but I’d be perfectly happy with just making Error.prepareStackTrace do the exact same thing as without source maps.

[targos]

#30046 is good to document how it works now, but I think that while we are figuring out how to interop Error.prepareStackTrace and --enable-source-maps, we should do this when --enable-source-maps is on:

• if Error.prepareStackTrace == the original function from V8: add source map information
• else: do not interfere and let the userland function do its thing

Is this technically possible?

[devsnek]

yeah, you would just add the source map stuff below those checks in errors.js. I just really don't like removing something as useful as source map info for this annoying legacy api.

[targos]

Sure, but the uses of the stack trace API that I have seen in the wild and block our flag for example in Express replace the function temporarily and put it back just after doing their thing so it wouldn't affect source map info

[Flarna]

Is there some other API available to get a non stringified stack trace?
As far as I know it's not possible to get typename/function name back from error.stack as you may get something like foo.bar.name and it's not clear which . actually splits type/function name.

[devsnek]

@Flarna not at the moment. There is https://github.com/tc39/proposal-error-stacks which is going to eventually be something, but not any time soon.

[Flarna]

I have not digged into the --enable-source-maps feature yet but seems I should do. So please excuse if my statements are naive/wrong.

FYI the use case we have in our monitoring regarding this: We correlate the function name, location seen on Error objects during e.g. HTTP requests with the function names seen by e.g CpuProfiler and what we get by asking a Function instance (either the name porperty or the inferred name).
We set Error.prepareStackTrace just to extract the Callsites from an Error and restore the original back.

As this correlation is done by using hashes the strings we extract have to match.

It would be really cool to get mapped info in all this cases if available. Seems the current implemenation effects only stack traces therefore the option to get it unmapped everywhere would be good.

[bcoe]

👋 prepareStackTrace() will now again work, even when --enable-source-maps is set:

#31143

There's also discussion in this PR about how to expose source maps for upstream tooling (would appreciate your thoughts).

@devsnek should we hold off on closing this until the next Node.js release I suppose?

[haggholm]

Will this be available in (backported to?) Node 12 LTS, or only in 13+?

(As someone currently stuck with 12, because it’s LTS, I think it would be lovely to see it backported, if necessary.)

[MylesBorins]

@haggholm I just checked and 9cdda60 lands cleanly on the v12.x-staging branch and is not marked as Semver-Major or Semver-Minor. As such I believe that it should get included in the branch once it has had time to bake in a 13.x release

[bcoe]

I have tested this fix in v12.16.0, this should no longer be an issue 👍