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

fix: source maps in Node v21.6.0+ & v20.12.0+ #518

Merged
merged 13 commits into from
Apr 4, 2024

Conversation

cenfun
Copy link
Contributor

@cenfun cenfun commented Apr 4, 2024

fix #506
fix #478
close #513
close #465

@privatenumber
Copy link
Owner

Can you pull in this in to see if it passes? #513

@cenfun
Copy link
Contributor Author

cenfun commented Apr 4, 2024

I don't know how to do it, but create PR here #519

// https://github.com/nodejs/node/pull/50827
// Allow using --enable-source-maps together with custom Error.prepareStackTrace
export const prepareStackTraceWithSourcemap: Version[] = [
[20, 12, 0],
Copy link
Owner

@privatenumber privatenumber Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// 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[] = [
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
export const prepareStackTraceWithSourcemap: Version[] = [
export const prepareStackTraceExposed: Version[] = [

@@ -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')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@@ -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')
Copy link
Owner

@privatenumber privatenumber Apr 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are you sure remove checking Error.prepareStackTrace? I don't know the history.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I dug into it deeper...

Previously, the Node docs said:

Overriding Error.prepareStackTrace prevents --enable-source-maps from modifying the stack trace.

And the prepareStackTrace logic was:

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed

@privatenumber
Copy link
Owner

I'm curious if this would also fix:

Will investigate in a bit

@cenfun
Copy link
Contributor Author

cenfun commented Apr 4, 2024

for #433 see my comments #433 (comment)

@privatenumber privatenumber changed the title fix issue 506 fix: source maps in Node v21.6.0+ & v20.12.0+ Apr 4, 2024
@privatenumber privatenumber merged commit 1a10da7 into privatenumber:develop Apr 4, 2024
1 of 2 checks passed
@privatenumber
Copy link
Owner

Thanks @cenfun—really appreciate you helping out!

Would love for us to continue collaborating in the future!

@privatenumber
Copy link
Owner

🎉 This issue has been resolved in v4.7.2

If you appreciate this project, please consider supporting this project by sponsoring ❤️ 🙏

@SuperchupuDev

This comment has been minimized.

Repository owner locked and limited conversation to collaborators Apr 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

VS Code breakpoint with tsx and Nodejs 20.12.0 does not work Source maps not working in Node 21.6 and above
3 participants