Skip to content

Commit fb2ccb1

Browse files
joyeecheungaduh95
authored andcommitted
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 0b3fbc8 commit fb2ccb1

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
@@ -1822,7 +1822,8 @@ function wrapSafe(filename, content, cjsModuleInstance, format) {
18221822
*/
18231823
Module.prototype._compile=function(content,filename,format){
18241824
if(format==='commonjs-typescript'||format==='module-typescript'||format==='typescript'){
1825-
content=stripTypeScriptModuleTypes(content,filename);
1825+
this[kURL]??=convertCJSFilenameToURL(filename);
1826+
content=stripTypeScriptModuleTypes(content,filename,this[kURL]);
18261827
switch(format){
18271828
case'commonjs-typescript': {
18281829
format='commonjs';

‎lib/internal/modules/typescript.js‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -175,9 +175,10 @@ function getCachedCodeType(mode, sourceMap) {
175175
* It is used by internal loaders.
176176
* @param {string} source TypeScript code to parse.
177177
* @param {string} filename The filename of the source code.
178+
* @param {string} [sourceURL] The source URL of the source code. If not specified, use filename.
178179
* @returns {TransformOutput} The stripped TypeScript code.
179180
*/
180-
functionstripTypeScriptModuleTypes(source,filename){
181+
functionstripTypeScriptModuleTypes(source,filename,sourceURL){
181182
assert(typeofsource==='string');
182183
if(isUnderNodeModules(filename)){
183184
thrownewERR_UNSUPPORTED_NODE_MODULES_TYPE_STRIPPING(filename);
@@ -203,7 +204,7 @@ function stripTypeScriptModuleTypes(source, filename) {
203204
constoptions={
204205
mode,
205206
sourceMap,
206-
filename,
207+
filename: sourceURL??filename,
207208
};
208209

209210
consttranspiled=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
constcommon=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
constfixtures=require('../common/fixtures');
913
constassert=require('assert');
10-
const{ pathToFileURL }=require('url');
1114

12-
constscriptPath=fixtures.path('typescript/ts/test-typescript.ts');
13-
constscriptURL=pathToFileURL(scriptPath);
14-
15-
asyncfunctionrunTest(){
16-
constchild=newNodeInstance(
17-
['--inspect-brk=0'],
18-
undefined,
19-
scriptPath);
15+
constscenarios=[
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+
asyncfunctioncollectParsedScripts(scriptPath){
58+
constchild=newNodeInstance(['--inspect-brk=0'],undefined,scriptPath);
2159
constsession=awaitchild.connectInspectorSession();
2260

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

32-
constscriptParsed=awaitsession.waitForNotification((notification)=>{
33-
if(notification.method!=='Debugger.scriptParsed')returnfalse;
34-
35-
returnnotification.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+
constscripts=newMap();
73+
awaitsession.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+
returnnotification.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-
awaitsession.waitForPauseOnStart();
41-
awaitsession.runToCompletion();
86+
awaitsession.waitForDisconnect();
4287

4388
assert.strictEqual((awaitchild.expectShutdown()).exitCode,0);
89+
returnscripts;
4490
}
4591

46-
runTest().then(common.mustCall());
92+
describe('type stripping source URLs',{concurrency: !process.env.TEST_PARALLEL},()=>{
93+
for(const{ entry, expected }ofscenarios){
94+
it(entry,async()=>{
95+
constscripts=awaitcollectParsedScripts(fixtures.path(entry));
96+
for(consthrefofexpected){
97+
constparams=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)