feat(node): Skip context lines lookup when source maps are enabled #17405
Conversation
size-limit report 📦
|
| // Nest.js always enabled this, without an easy way for us to detect this | ||
| // so we just enable it by default | ||
| // see: https://github.com/nestjs/nest-cli/blob/f5dbb573df1fe103139026a36b6d0efe65e8e985/actions/start.action.ts#L220 | ||
| hasSourceMaps: true, |
There was a problem hiding this comment.
m: I'm not sure if this is really always the case. I could imagine a deployment config where a nest app isn't started via the Nest CLI but directly via node.In which case, this probably doesn't work as we'd expect it 🤔 Might make sense to check official NestJS deployment guides to be safe.
There was a problem hiding this comment.
Looks like the CLI isn't meant to start a Nest app in prod: https://docs.nestjs.com/deployment#node_envproduction
There was a problem hiding this comment.
hmmm, good point! 😬
I haven't found a reliably way to figure out if this flag is set in Nest.js 😢 I can't seem to get a hold of these args/env vars in nest, probably due to how this is spawned (I suppose?)... I'll think about it some more. Generally speaking, this should mostly be not too dangerous either way, as the presence of .ts file more or less indicates that this is most likely source mapped already anyhow, but it is less safe 🤔
| function shouldSkipContextLinesThatAreAlreadySourceMapped(path: string, frame: StackFrame): boolean { | ||
| // For non-in-app frames, we skip context lines when we are reasonably certain that the path is already sourcemapped. | ||
| // For now, we only consider .ts files because they can never appear otherwise in a stackframe, if not already sourcemapped. | ||
| if (frame.in_app === false && path.endsWith('.ts')) { |
There was a problem hiding this comment.
super-l: We could probably match against /.(c|m)?tsx?/ to cover some more TS file variations. Happy to leave this up to you though
| if (frame.in_app === false && path.endsWith('.ts')) { | |
| if (frame.in_app === false && path.endsWith('.ts')) { |
There was a problem hiding this comment.
yeah good point, likely a good idea 👍
This improves lookup of files for the node context lines integration, when
--enable-source-mapsis used.With this setting, error stacks will already be sourcemapped. This can lead to file lookups repeatedly failing, because we try to load files that do not exist in node modules. To avoid this, when we detect that this setting exists, we skip this in cases where we feel confident that this is unnecessary. For now, this is only one case:
In this case, we'll skip trying to lookup the file.
We determine if the setting is defined by looking at
process.env.NODE_OPTIONSandprocess.argv.Additionally, in nest.js this is always defined, so we always handle it this way there.
Additionally, I also adjusted the log message when we fail to lookup the file, as this looked pretty ominous and unclear how problematic that was, even if this is not really an error per-se.