fix: source maps in Node v21.6.0+ & v20.12.0+ #518
Conversation
|
Can you pull in this in to see if it passes? #513 |
|
I don't know how to do it, but create PR here #519 |
src/utils/node-features.ts
Outdated
| // https://github.com/nodejs/node/pull/50827 | ||
| // Allow using --enable-source-maps together with custom Error.prepareStackTrace | ||
| export const prepareStackTraceWithSourcemap: Version[] = [ | ||
| [20, 12, 0], |
There was a problem hiding this comment.
Also 21.6.0
Seems nodejs/node#50827 is included in the following releases (ordered by release date):
v21.6.0 nodejs/node@291265ce27
v21.6.1 nodejs/node@291265ce27
v21.6.2 nodejs/node@291265ce27
v21.7.0 nodejs/node@291265ce27
v21.7.1 nodejs/node@291265ce27
v20.12.0 nodejs/node@f4d7f0498e
v21.7.2 nodejs/node@291265ce27
v20.12.1 nodejs/node@f4d7f0498e
src/utils/node-features.ts
Outdated
| // https://nodejs.org/docs/latest-v20.x/api/cli.html#--enable-source-maps | ||
| // https://github.com/nodejs/node/pull/50827 | ||
| // Allow using --enable-source-maps together with custom Error.prepareStackTrace | ||
| export const prepareStackTraceWithSourcemap: Version[] = [ |
There was a problem hiding this comment.
| export const prepareStackTraceWithSourcemap: Version[] = [ | |
| export const prepareStackTraceExposed: Version[] = [ |
src/source-map.ts
Outdated
| @@ -18,7 +19,7 @@ export const installSourceMapSupport = () => { | |||
| * | |||
| * https://github.com/nodejs/node/blob/91193825551f9301b6ab52d96211b38889149892/lib/internal/errors.js#L141 | |||
| */ | |||
| && typeof Error.prepareStackTrace !== 'function' | |||
| && (isFeatureSupported(prepareStackTraceWithSourcemap) || typeof Error.prepareStackTrace !== 'function') | |||
src/source-map.ts
Outdated
| @@ -18,7 +19,7 @@ export const installSourceMapSupport = () => { | |||
| * | |||
| * https://github.com/nodejs/node/blob/91193825551f9301b6ab52d96211b38889149892/lib/internal/errors.js#L141 | |||
| */ | |||
| && typeof Error.prepareStackTrace !== 'function' | |||
| && (isFeatureSupported(prepareStackTraceExposed) || typeof Error.prepareStackTrace !== 'function') | |||
There was a problem hiding this comment.
Upon reading what the default prepareStackTrace looks like, I'm okay with removing this check entirely.
The Node docs previously implied that if it was overridden sourcemaps wouldn't work expectedly, but the implementation is actually so simple that the default behavior can easily be replicated from the user-end.
There was a problem hiding this comment.
are you sure remove checking Error.prepareStackTrace? I don't know the history.
There was a problem hiding this comment.
Hmm, I dug into it deeper...
Previously, the Node docs said:
Overriding
Error.prepareStackTraceprevents--enable-source-mapsfrom modifying the stack trace.
And the prepareStackTrace logic was:
- no source maps: https://github.com/nodejs/node/blob/v20.11.1/lib/internal/errors.js#L119-L133
- source maps: https://github.com/nodejs/node/blob/v20.11.1/lib/internal/source_map/prepare_stack_trace.js#L32-L110
Not generating source maps when Error.prepareStackTrace was set was a performance optimization because I assumed the custom prepareStackTrace wouldn't be able to handle the source map. But this may have been an incorrect assumption. Furthermore, it's a rare optimization because I think Error.prepareStackTrace is not very common.
That said, it's probably fine to remove this check entirely for how little benefit it provides, and the possibility that a custom prepareStackTrace can handle source maps.
|
I'm curious if this would also fix:
Will investigate in a bit |
|
for #433 see my comments #433 (comment) |
privatenumber
changed the title
|
Thanks @cenfun—really appreciate you helping out! Would love for us to continue collaborating in the future! |
|
🎉 This issue has been resolved in v4.7.2 If you appreciate this project, please consider supporting this project by sponsoring ❤️ 🙏 |
This comment has been minimized.
This comment has been minimized.
fix #506
fix #478
close #513
close #465