Skip to content

Commit 1654684

Browse files
authored
esm: ensure watch mode restarts after syntax errors
Move watch dependency reporting earlier in module resolution to ensure file dependencies are tracked even when parsing fails. Fixes: #61153 PR-URL: #61232 Reviewed-By: Aviv Keller <me@aviv.sh> Reviewed-By: Moshe Atlow <moshe@atlow.co.il> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Juan José Arboleda <soyjuanarbol@gmail.com>
1 parent 9575913 commit 1654684

File tree

2 files changed

+132
-5
lines changed

2 files changed

+132
-5
lines changed

lib/internal/modules/esm/loader.js

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -542,6 +542,12 @@ class ModuleLoader {
542542
*/
543543
#getOrCreateModuleJobAfterResolve(parentURL, resolveResult, request, requestType) {
544544
const { url, format } = resolveResult;
545+
546+
if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) {
547+
const type = requestType === kRequireInImportedCJS ? 'require' : 'import';
548+
process.send({ [`watch:${type}`]: [url] });
549+
}
550+
545551
// TODO(joyeecheung): update the module requests to use importAttributes as property names.
546552
const importAttributes = resolveResult.importAttributes ?? request.attributes;
547553
let job = this.loadCache.get(url, importAttributes.type);
@@ -570,11 +576,6 @@ class ModuleLoader {
570576
assert(moduleOrModulePromise instanceof ModuleWrap, `Expected ModuleWrap for loading ${url}`);
571577
}
572578

573-
if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) {
574-
const type = requestType === kRequireInImportedCJS ? 'require' : 'import';
575-
process.send({ [`watch:${type}`]: [url] });
576-
}
577-
578579
const { ModuleJob, ModuleJobSync } = require('internal/modules/esm/module_job');
579580
// TODO(joyeecheung): use ModuleJobSync for kRequireInImportedCJS too.
580581
const ModuleJobCtor = (requestType === kImportInRequiredESM ? ModuleJobSync : ModuleJob);
Lines changed: 126 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,126 @@
1+
import * as common from '../common/index.mjs';
2+
import tmpdir from '../common/tmpdir.js';
3+
import assert from 'node:assert';
4+
import path from 'node:path';
5+
import { execPath } from 'node:process';
6+
import { spawn } from 'node:child_process';
7+
import { writeFileSync } from 'node:fs';
8+
import { inspect } from 'node:util';
9+
import { createInterface } from 'node:readline';
10+
11+
if (common.isIBMi)
12+
common.skip('IBMi does not support `fs.watch()`');
13+
14+
let tmpFiles = 0;
15+
function createTmpFile(content = 'console.log("running");', ext = '.js', basename = tmpdir.path) {
16+
const file = path.join(basename, `${tmpFiles++}${ext}`);
17+
writeFileSync(file, content);
18+
return file;
19+
}
20+
21+
function runInBackground({ args = [], options = {}, completed = 'Completed running', shouldFail = false }) {
22+
let future = Promise.withResolvers();
23+
let child;
24+
let stderr = '';
25+
let stdout = [];
26+
27+
const run = () => {
28+
args.unshift('--no-warnings');
29+
child = spawn(execPath, args, { encoding: 'utf8', stdio: 'pipe', ...options });
30+
31+
child.stderr.on('data', (data) => {
32+
stderr += data;
33+
});
34+
35+
const rl = createInterface({ input: child.stdout });
36+
rl.on('line', (data) => {
37+
if (!data.startsWith('Waiting for graceful termination') && !data.startsWith('Gracefully restarted')) {
38+
stdout.push(data);
39+
if (data.startsWith(completed)) {
40+
future.resolve({ stderr, stdout });
41+
future = Promise.withResolvers();
42+
stdout = [];
43+
stderr = '';
44+
} else if (data.startsWith('Failed running')) {
45+
if (shouldFail) {
46+
future.resolve({ stderr, stdout });
47+
} else {
48+
future.reject({ stderr, stdout });
49+
}
50+
future = Promise.withResolvers();
51+
stdout = [];
52+
stderr = '';
53+
}
54+
}
55+
});
56+
};
57+
58+
return {
59+
async done() {
60+
child?.kill();
61+
future.resolve();
62+
return { stdout, stderr };
63+
},
64+
restart(timeout = 1000) {
65+
if (!child) {
66+
run();
67+
}
68+
const timer = setTimeout(() => {
69+
if (!future.resolved) {
70+
child.kill();
71+
future.reject(new Error('Timed out waiting for restart'));
72+
}
73+
}, timeout);
74+
return future.promise.finally(() => {
75+
clearTimeout(timer);
76+
});
77+
},
78+
};
79+
}
80+
81+
tmpdir.refresh();
82+
83+
// Create initial file with valid code
84+
const initialContent = `console.log('hello, world');`;
85+
const file = createTmpFile(initialContent, '.mjs');
86+
87+
const { done, restart } = runInBackground({
88+
args: ['--watch', file],
89+
completed: 'Completed running',
90+
shouldFail: true,
91+
});
92+
93+
try {
94+
const { stdout, stderr } = await restart();
95+
assert.strictEqual(stderr, '');
96+
assert.deepStrictEqual(stdout, [
97+
'hello, world',
98+
`Completed running ${inspect(file)}. Waiting for file changes before restarting...`,
99+
]);
100+
101+
// Update file with syntax error
102+
const syntaxErrorContent = `console.log('hello, wor`;
103+
writeFileSync(file, syntaxErrorContent);
104+
105+
// Wait for the failed restart
106+
const { stderr: stderr2, stdout: stdout2 } = await restart();
107+
assert.match(stderr2, /SyntaxError: Invalid or unexpected token/);
108+
assert.deepStrictEqual(stdout2, [
109+
`Restarting ${inspect(file)}`,
110+
`Failed running ${inspect(file)}. Waiting for file changes before restarting...`,
111+
]);
112+
113+
writeFileSync(file, `console.log('hello again, world');`);
114+
115+
const { stderr: stderr3, stdout: stdout3 } = await restart();
116+
117+
// Verify it recovered and ran successfully
118+
assert.strictEqual(stderr3, '');
119+
assert.deepStrictEqual(stdout3, [
120+
`Restarting ${inspect(file)}`,
121+
'hello again, world',
122+
`Completed running ${inspect(file)}. Waiting for file changes before restarting...`,
123+
]);
124+
} finally {
125+
await done();
126+
}

0 commit comments

Comments
 (0)