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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 13 commits into from
Apr 4, 2024
Merged

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

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
999b9a9
wip
privatenumber Apr 1, 2024
49645e0
wip
privatenumber Apr 1, 2024
8495eff
wip
privatenumber Apr 1, 2024
33f6c64
fix issue 506
cenfun Apr 4, 2024
b6ef830
Merge remote-tracking branch 'origin/test-latest-20' into issues-506
cenfun Apr 4, 2024
4560ee9
add 21, 6, 0
cenfun Apr 4, 2024
2249ec6
Merge branch 'develop' into issues-506
privatenumber Apr 4, 2024
b1f23c8
add eol
cenfun Apr 4, 2024
b2bd9e0
Merge branch 'issues-506' of https://github.com/cenfun/tsx into issue…
cenfun Apr 4, 2024
1d5966b
test v21
privatenumber Apr 4, 2024
19ea651
remove checking prepareStackTrace
cenfun Apr 4, 2024
b6198ca
styling
privatenumber Apr 4, 2024
218e9ee
comment
privatenumber Apr 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .nvmrc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
20.10.0
v20.12.0
3 changes: 2 additions & 1 deletion src/source-map.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import type { Transformed } from './utils/transform/apply-transformers.js';
import { isFeatureSupported, prepareStackTraceExposed } from './utils/node-features.js';

const inlineSourceMapPrefix = '\n//# sourceMappingURL=data:application/json;base64,';

Expand All @@ -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 •
edited
Loading

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

);

if (hasNativeSourceMapSupport) {
Expand Down
8 changes: 8 additions & 0 deletions src/utils/node-features.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,3 +53,11 @@ export const importAttributes: Version[] = [
export const testRunnerGlob: Version[] = [
[21, 0, 0],
];

// 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 prepareStackTraceExposed: Version[] = [
[20, 12, 0],
Copy link
Owner

@privatenumber privatenumber Apr 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

[21, 6, 0],
];
5 changes: 3 additions & 2 deletions tests/utils/node-versions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ export const nodeVersions = [
&& process.platform !== 'win32'
)
? [
'21.5.0',
latestMajor('20.10.0'),
latestMajor('21.7.2'),
'21.0.0',
latestMajor('20.12.0'),
'20.0.0',
latestMajor('18.20.0'),
'18.0.0',
Expand Down
Loading