Skip to content

Commit 4219808

Browse files
authored
module: use file: URL as sourceURL for type-stripped CommonJS
The CommonJS loader was passing the bare filesystem path as the `//# sourceURL` comment of type-stripped TypeScript, this leads to two problems: 1. It reports hasSourceURL = false if the path contains any whitespaces as this breaks V8's magic comment parser. 2. The inspector would incorrectly report a file path verbatim as the script's URL. Pass the module's file: URL as the sourceURL so the reported URL is consistent across loaders and won't lead to an issue in inspector clients that actually except a real URL for the scripts. Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com> PR-URL: #63705 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Marco Ippolito <marcoippolito54@gmail.com> Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
1 parent 09c603f commit 4219808

3 files changed

Lines changed: 83 additions & 22 deletions

File tree

lib/internal/modules/cjs/loader.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1830,7 +1830,8 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
18301830
*/
18311831
Module.prototype._compile = function(content, filename, format) {
18321832
if (format === 'commonjs-typescript' || format === 'module-typescript' || format === 'typescript') {
1833-
content = stripTypeScriptModuleTypes(content, filename);
1833+
this[kURL] ??= convertCJSFilenameToURL(filename);
1834+
content = stripTypeScriptModuleTypes(content, filename, this[kURL]);
18341835
switch (format) {
18351836
case 'commonjs-typescript': {
18361837
format = 'commonjs';

lib/internal/modules/typescript.js

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -156,9 +156,10 @@ function stripTypeScriptTypesForCoverage(code) {
156156
* It is used by internal loaders.
157157
* @param {string} source TypeScript code to parse.
158158
* @param {string} filename The filename of the source code.
159+
* @param {string} [sourceURL] The source URL of the source code. If not specified, use filename.
159160
* @returns {TransformOutput} The stripped TypeScript code.
160161
*/
161-
function stripTypeScriptModuleTypes(source, filename) {
162+
function stripTypeScriptModuleTypes(source, filename, sourceURL) {
162163
assert(typeof source === 'string');
163164
if (isUnderNodeModules(filename)) {
164165
throw new ERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING(filename);
@@ -178,7 +179,7 @@ function stripTypeScriptModuleTypes(source, filename) {
178179
const options = {
179180
mode: 'strip-only',
180181
sourceMap,
181-
filename,
182+
filename: sourceURL ?? filename,
182183
};
183184

184185
const transpiled = processTypeScriptCode(source, options);
Lines changed: 78 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,61 @@
1+
// Verifies that type-stripped TypeScript reports a file: URL as its script
2+
// sourceURL to the inspector across all module loading paths, hinting that it
3+
// is a generated source.
14
'use strict';
25

36
const common = require('../common');
7+
const { describe, it } = require('node:test');
48
common.skipIfInspectorDisabled();
59
if (!process.config.variables.node_use_amaro) common.skip('Requires Amaro');
610

711
const { NodeInstance } = require('../common/inspector-helper.js');
812
const fixtures = require('../common/fixtures');
913
const assert = require('assert');
10-
const { pathToFileURL } = require('url');
1114

12-
const scriptPath = fixtures.path('typescript/ts/test-typescript.ts');
13-
const scriptURL = pathToFileURL(scriptPath);
14-
15-
async function runTest() {
16-
const child = new NodeInstance(
17-
['--inspect-brk=0'],
18-
undefined,
19-
scriptPath);
15+
const scenarios = [
16+
{
17+
// CommonJS: a .ts entry point.
18+
entry: 'typescript/ts/test-typescript.ts',
19+
expected: [
20+
fixtures.fileURL('typescript/ts/test-typescript.ts').href,
21+
],
22+
},
23+
{
24+
// CommonJS: a .ts entry that require()s a .cts dependency.
25+
entry: 'typescript/ts/test-require-cts.ts',
26+
expected: [
27+
fixtures.fileURL('typescript/ts/test-require-cts.ts').href,
28+
fixtures.fileURL('typescript/cts/test-cts-export-foo.cts').href,
29+
],
30+
},
31+
{
32+
// CommonJS: a .ts entry that require()s a .mts dependency.
33+
entry: 'typescript/ts/test-require-mts.ts',
34+
expected: [
35+
fixtures.fileURL('typescript/ts/test-require-mts.ts').href,
36+
fixtures.fileURL('typescript/mts/test-mts-export-foo.mts').href,
37+
],
38+
},
39+
{
40+
// ESM: a .mts entry that imports a .ts dependency.
41+
entry: 'typescript/mts/test-import-ts-file.mts',
42+
expected: [
43+
fixtures.fileURL('typescript/mts/test-import-ts-file.mts').href,
44+
fixtures.fileURL('typescript/mts/test-module-export.ts').href,
45+
],
46+
},
47+
{
48+
// ESM: a .mts entry that imports a .cts dependency.
49+
entry: 'typescript/mts/test-import-commonjs.mts',
50+
expected: [
51+
fixtures.fileURL('typescript/mts/test-import-commonjs.mts').href,
52+
fixtures.fileURL('typescript/cts/test-cts-export-foo.cts').href,
53+
],
54+
},
55+
];
2056

57+
async function collectParsedScripts(scriptPath) {
58+
const child = new NodeInstance(['--inspect-brk=0'], undefined, scriptPath);
2159
const session = await child.connectInspectorSession();
2260

2361
await session.send({ method: 'NodeRuntime.enable' });
@@ -29,18 +67,39 @@ async function runTest() {
2967
]);
3068
await session.send({ method: 'NodeRuntime.disable' });
3169

32-
const scriptParsed = await session.waitForNotification((notification) => {
33-
if (notification.method !== 'Debugger.scriptParsed') return false;
34-
35-
return notification.params.url === scriptPath || notification.params.url === scriptURL.href;
70+
// Collect every parsed script while resuming through the break on start and
71+
// any later pauses, until the main execution context is destroyed.
72+
const scripts = new Map();
73+
await session.waitForNotification((notification) => {
74+
if (notification.method === 'Debugger.scriptParsed') {
75+
const { url } = notification.params;
76+
if (!url.startsWith('node:')) {
77+
scripts.set(url, notification.params);
78+
}
79+
}
80+
if (notification.method === 'Debugger.paused') {
81+
session.send({ method: 'Debugger.resume' });
82+
}
83+
return notification.method === 'Runtime.executionContextDestroyed' &&
84+
notification.params.executionContextId === 1;
3685
});
37-
// Verify that the script has a sourceURL, hinting that it is a generated source.
38-
assert(scriptParsed.params.hasSourceURL || common.isInsideDirWithUnusualChars);
39-
40-
await session.waitForPauseOnStart();
41-
await session.runToCompletion();
86+
await session.waitForDisconnect();
4287

4388
assert.strictEqual((await child.expectShutdown()).exitCode, 0);
89+
return scripts;
4490
}
4591

46-
runTest().then(common.mustCall());
92+
describe('type stripping source URLs', { concurrency: !process.env.TEST_PARALLEL }, () => {
93+
for (const { entry, expected } of scenarios) {
94+
it(entry, async () => {
95+
const scripts = await collectParsedScripts(fixtures.path(entry));
96+
for (const href of expected) {
97+
const params = scripts.get(href);
98+
assert(params,
99+
`expected ${entry} to report ${href} to the inspector, ` +
100+
`got:\n- ${[...scripts.keys()].join('\n- ')}`);
101+
assert(params.hasSourceURL);
102+
}
103+
});
104+
}
105+
});

0 commit comments

Comments
 (0)