From e8fe5bd529e0e44744c369302ee6190bf98f4c81 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sun, 6 Nov 2022 23:22:52 +0200 Subject: [PATCH 1/5] watch: watch for missing dependencies --- lib/internal/modules/cjs/loader.js | 10 +++- lib/internal/modules/esm/loader.js | 2 +- lib/internal/modules/esm/resolve.js | 3 ++ lib/internal/watch_mode/files_watcher.js | 11 +++-- test/fixtures/watch-mode/ipc.js | 8 +-- test/sequential/test-watch-mode.mjs | 63 ++++++++++++++++++++---- 6 files changed, 77 insertions(+), 20 deletions(-) diff --git a/lib/internal/modules/cjs/loader.js b/lib/internal/modules/cjs/loader.js index 02a07222746dfd..340df162a4464f 100644 --- a/lib/internal/modules/cjs/loader.js +++ b/lib/internal/modules/cjs/loader.js @@ -28,6 +28,7 @@ const { ArrayPrototypeIncludes, ArrayPrototypeIndexOf, ArrayPrototypeJoin, + ArrayPrototypeMap, ArrayPrototypePush, ArrayPrototypeSlice, ArrayPrototypeSplice, @@ -191,7 +192,13 @@ function updateChildren(parent, child, scan) { function reportModuleToWatchMode(filename) { if (shouldReportRequiredModules && process.send) { - process.send({ 'watch:require': filename }); + process.send({ 'watch:require': [filename] }); + } +} + +function reportModuleNotFoundToWatchMode(basePath, extensions) { + if (shouldReportRequiredModules && process.send) { + process.send({ 'watch:require': ArrayPrototypeMap(extensions, (ext) => path.resolve(`${basePath}${ext}`)) }); } } @@ -648,6 +655,7 @@ Module._findPath = function(request, paths, isMain) { Module._pathCache[cacheKey] = filename; return filename; } + reportModuleNotFoundToWatchMode(basePath, ArrayPrototypeConcat([''], exts)); } return false; diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js index 55c17d97ec5695..e4d320ebc39cce 100644 --- a/lib/internal/modules/esm/loader.js +++ b/lib/internal/modules/esm/loader.js @@ -463,7 +463,7 @@ class ESMLoader { ); if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { - process.send({ 'watch:import': url }); + process.send({ 'watch:import': [url] }); } const job = new ModuleJob( diff --git a/lib/internal/modules/esm/resolve.js b/lib/internal/modules/esm/resolve.js index 561bbbe32e626e..e4ad685a6938e7 100644 --- a/lib/internal/modules/esm/resolve.js +++ b/lib/internal/modules/esm/resolve.js @@ -254,6 +254,9 @@ function finalizeResolution(resolved, base, preserveSymlinks) { err.url = String(resolved); throw err; } else if (!stats.isFile()) { + if (process.env.WATCH_REPORT_DEPENDENCIES && process.send) { + process.send({ 'watch:require': [path || resolved.pathname] }); + } throw new ERR_MODULE_NOT_FOUND( path || resolved.pathname, base && fileURLToPath(base), 'module'); } diff --git a/lib/internal/watch_mode/files_watcher.js b/lib/internal/watch_mode/files_watcher.js index 6c6c0f27fd8f8d..f2141051fceaf4 100644 --- a/lib/internal/watch_mode/files_watcher.js +++ b/lib/internal/watch_mode/files_watcher.js @@ -1,6 +1,8 @@ 'use strict'; const { + ArrayIsArray, + ArrayPrototypeForEach, SafeMap, SafeSet, StringPrototypeStartsWith, @@ -94,6 +96,7 @@ class FilesWatcher extends EventEmitter { } filterFile(file) { + if (!file) return; if (supportsRecursiveWatching) { this.watchPath(dirname(file)); } else { @@ -109,11 +112,11 @@ class FilesWatcher extends EventEmitter { } child.on('message', (message) => { try { - if (message['watch:require']) { - this.filterFile(message['watch:require']); + if (ArrayIsArray(message['watch:require'])) { + ArrayPrototypeForEach(message['watch:require'], (file) => this.filterFile(file)); } - if (message['watch:import']) { - this.filterFile(fileURLToPath(message['watch:import'])); + if (ArrayIsArray(message['watch:import'])) { + ArrayPrototypeForEach(message['watch:import'], (file) => this.filterFile(fileURLToPath(file))); } } catch { // Failed watching file. ignore diff --git a/test/fixtures/watch-mode/ipc.js b/test/fixtures/watch-mode/ipc.js index 021df9973511d0..5881299387e5b4 100644 --- a/test/fixtures/watch-mode/ipc.js +++ b/test/fixtures/watch-mode/ipc.js @@ -6,7 +6,7 @@ const tmpdir = require('../../common/tmpdir'); const tmpfile = path.join(tmpdir.path, 'file'); fs.writeFileSync(tmpfile, ''); -process.send({ 'watch:require': path.resolve(__filename) }); -process.send({ 'watch:import': url.pathToFileURL(path.resolve(__filename)).toString() }); -process.send({ 'watch:import': url.pathToFileURL(tmpfile).toString() }); -process.send({ 'watch:import': new URL('http://invalid.com').toString() }); +process.send({ 'watch:require': [path.resolve(__filename)] }); +process.send({ 'watch:import': [url.pathToFileURL(path.resolve(__filename)).toString()] }); +process.send({ 'watch:import': [url.pathToFileURL(tmpfile).toString()] }); +process.send({ 'watch:import': [new URL('http://invalid.com').toString()] }); diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index e24fb97ad234a8..8e663e60eb682e 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -13,6 +13,12 @@ import { once } from 'node:events'; if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); +function deferred() { + let res; + const promise = new Promise((resolve) => res = resolve); + return { resolve: res, promise }; +} + function restart(file) { // To avoid flakiness, we save the file repeatedly until test is done writeFileSync(file, readFileSync(file)); @@ -59,8 +65,8 @@ async function spawnWithRestarts({ } let tmpFiles = 0; -function createTmpFile(content = 'console.log("running");') { - const file = path.join(tmpdir.path, `${tmpFiles++}.js`); +function createTmpFile(content = 'console.log("running");', ext = '.js') { + const file = path.join(tmpdir.path, `${tmpFiles++}${ext}`); writeFileSync(file, content); return file; } @@ -74,6 +80,25 @@ function assertRestartedCorrectly({ stdout, messages: { inner, completed, restar assert.deepStrictEqual(lines.slice(-end.length), end); } +async function failWriteSucceed({ file, watchedFile }) { + let stdout = ''; + const notFound = deferred(); + const completed = deferred(); + const child = spawn(execPath, ['--watch', '--no-warnings', file], { encoding: 'utf8' }); + + child.stdout.on('data', (data) => { + stdout += data; + if (data.toString().startsWith('Failed running')) notFound.resolve(); + if (data.toString().startsWith('Completed running')) completed.resolve(); + }); + + await notFound.promise; + writeFileSync(watchedFile, 'console.log("test has ran");'); + await completed.promise; + child.kill(); + assert.match(stdout, /test has ran/); +} + tmpdir.refresh(); // Warning: this suite can run safely with concurrency: true @@ -104,14 +129,6 @@ describe('watch mode', { concurrency: true, timeout: 60_0000 }, () => { }); }); - it('should not watch when running an non-existing file', async () => { - const file = fixtures.path('watch-mode/non-existing.js'); - const { stderr, stdout } = await spawnWithRestarts({ file, restarts: 0 }); - - assert.match(stderr, /code: 'MODULE_NOT_FOUND'/); - assert.strictEqual(stdout, `Failed running ${inspect(file)}\n`); - }); - it('should watch when running an non-existing file - when specified under --watch-path', { skip: !common.isOSX && !common.isWindows }, async () => { @@ -220,4 +237,30 @@ describe('watch mode', { concurrency: true, timeout: 60_0000 }, () => { messages: { restarted: `Restarting ${inspect(file)}`, completed: `Completed running ${inspect(file)}` }, }); }); + + it('should not watch when running an missing file', async () => { + const nonExistingfile = path.join(tmpdir.path, `${tmpFiles++}.js`); + await failWriteSucceed({ file: nonExistingfile, watchedFile: nonExistingfile }); + }); + + it('should not watch when running an missing mjs file', async () => { + const nonExistingfile = path.join(tmpdir.path, `${tmpFiles++}.mjs`); + await failWriteSucceed({ file: nonExistingfile, watchedFile: nonExistingfile }); + }); + + it('should watch changes to previously missing dependency', async () => { + const dependency = path.join(tmpdir.path, `${tmpFiles++}.js`); + const relativeDependencyPath = `./${path.basename(dependency)}`; + const dependant = createTmpFile(`console.log(require('${relativeDependencyPath}'))`); + + await failWriteSucceed({ file: dependant, watchedFile: dependency }); + }); + + it('should watch changes to previously missing ESM dependency', async () => { + const dependency = path.join(tmpdir.path, `${tmpFiles++}.mjs`); + const relativeDependencyPath = `./${path.basename(dependency)}`; + const dependant = createTmpFile(`import '${relativeDependencyPath}'`, '.mjs'); + + await failWriteSucceed({ file: dependant, watchedFile: dependency }); + }); }); From 27a6af3cdd0e7872233513c27391f2fa3e6ec606 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 10 Nov 2022 23:20:39 +0200 Subject: [PATCH 2/5] fix --- test/sequential/test-watch-mode.mjs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 8e663e60eb682e..eb29fd66291507 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -103,7 +103,7 @@ tmpdir.refresh(); // Warning: this suite can run safely with concurrency: true // only if tests do not watch/depend on the same files -describe('watch mode', { concurrency: true, timeout: 60_0000 }, () => { +describe('watch mode', { concurrency: true, timeout: 60_000 }, () => { it('should watch changes to a file - event loop ended', async () => { const file = createTmpFile(); const { stderr, stdout } = await spawnWithRestarts({ file }); From d4c49a69923054305b7da4db3dffc58fad4d812f Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Thu, 10 Nov 2022 23:21:00 +0200 Subject: [PATCH 3/5] debug --- test/sequential/test-watch-mode.mjs | 1 + 1 file changed, 1 insertion(+) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index eb29fd66291507..d84f83277b9b09 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -87,6 +87,7 @@ async function failWriteSucceed({ file, watchedFile }) { const child = spawn(execPath, ['--watch', '--no-warnings', file], { encoding: 'utf8' }); child.stdout.on('data', (data) => { + console.log(data.toString()); stdout += data; if (data.toString().startsWith('Failed running')) notFound.resolve(); if (data.toString().startsWith('Completed running')) completed.resolve(); From d1c155c7f33d49c3a2fe391802d921d05a6f92bc Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 12 Nov 2022 20:39:32 +0200 Subject: [PATCH 4/5] CR & fix --- test/sequential/test-watch-mode.mjs | 56 +++++++++++++++-------------- 1 file changed, 30 insertions(+), 26 deletions(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index d84f83277b9b09..65466f37394931 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -9,15 +9,12 @@ import { spawn } from 'node:child_process'; import { writeFileSync, readFileSync } from 'node:fs'; import { inspect } from 'node:util'; import { once } from 'node:events'; +import { createInterface } from 'node:readline/promises'; if (common.isIBMi) common.skip('IBMi does not support `fs.watch()`'); -function deferred() { - let res; - const promise = new Promise((resolve) => res = resolve); - return { resolve: res, promise }; -} +const supportsRecursive = common.isOSX || common.isWindows; function restart(file) { // To avoid flakiness, we save the file repeatedly until test is done @@ -81,23 +78,21 @@ function assertRestartedCorrectly({ stdout, messages: { inner, completed, restar } async function failWriteSucceed({ file, watchedFile }) { - let stdout = ''; - const notFound = deferred(); - const completed = deferred(); const child = spawn(execPath, ['--watch', '--no-warnings', file], { encoding: 'utf8' }); - child.stdout.on('data', (data) => { - console.log(data.toString()); - stdout += data; - if (data.toString().startsWith('Failed running')) notFound.resolve(); - if (data.toString().startsWith('Completed running')) completed.resolve(); - }); - - await notFound.promise; - writeFileSync(watchedFile, 'console.log("test has ran");'); - await completed.promise; - child.kill(); - assert.match(stdout, /test has ran/); + try { + // break the chunks into lines + for await(const data of createInterface({ input: child.stdout })) { + if (data.startsWith('Completed running')) { + break; + } + if (data.startsWith('Failed running')) { + writeFileSync(watchedFile, 'console.log("test has ran");'); + } + }; + } finally { + child.kill(); + } } tmpdir.refresh(); @@ -131,7 +126,7 @@ describe('watch mode', { concurrency: true, timeout: 60_000 }, () => { }); it('should watch when running an non-existing file - when specified under --watch-path', { - skip: !common.isOSX && !common.isWindows + skip: !supportsRecursive }, async () => { const file = fixtures.path('watch-mode/subdir/non-existing.js'); const watchedFile = fixtures.path('watch-mode/subdir/file.js'); @@ -238,18 +233,25 @@ describe('watch mode', { concurrency: true, timeout: 60_000 }, () => { messages: { restarted: `Restarting ${inspect(file)}`, completed: `Completed running ${inspect(file)}` }, }); }); - - it('should not watch when running an missing file', async () => { + + // TODO: Remove skip after https://github.com/nodejs/node/pull/45271 lands + it('should not watch when running an missing file', { + skip: !supportsRecursive + }, async () => { const nonExistingfile = path.join(tmpdir.path, `${tmpFiles++}.js`); await failWriteSucceed({ file: nonExistingfile, watchedFile: nonExistingfile }); }); - it('should not watch when running an missing mjs file', async () => { + it('should not watch when running an missing mjs file', { + skip: !supportsRecursive + }, async () => { const nonExistingfile = path.join(tmpdir.path, `${tmpFiles++}.mjs`); await failWriteSucceed({ file: nonExistingfile, watchedFile: nonExistingfile }); }); - it('should watch changes to previously missing dependency', async () => { + it('should watch changes to previously missing dependency', { + skip: !supportsRecursive + }, async () => { const dependency = path.join(tmpdir.path, `${tmpFiles++}.js`); const relativeDependencyPath = `./${path.basename(dependency)}`; const dependant = createTmpFile(`console.log(require('${relativeDependencyPath}'))`); @@ -257,7 +259,9 @@ describe('watch mode', { concurrency: true, timeout: 60_000 }, () => { await failWriteSucceed({ file: dependant, watchedFile: dependency }); }); - it('should watch changes to previously missing ESM dependency', async () => { + it('should watch changes to previously missing ESM dependency', { + skip: !supportsRecursive + }, async () => { const dependency = path.join(tmpdir.path, `${tmpFiles++}.mjs`); const relativeDependencyPath = `./${path.basename(dependency)}`; const dependant = createTmpFile(`import '${relativeDependencyPath}'`, '.mjs'); From 17f1fa264943d52539898e9923e3f90aa04a5bfc Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sat, 12 Nov 2022 20:48:44 +0200 Subject: [PATCH 5/5] lint --- test/sequential/test-watch-mode.mjs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 65466f37394931..9b894922621e3b 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -81,15 +81,15 @@ async function failWriteSucceed({ file, watchedFile }) { const child = spawn(execPath, ['--watch', '--no-warnings', file], { encoding: 'utf8' }); try { - // break the chunks into lines - for await(const data of createInterface({ input: child.stdout })) { - if (data.startsWith('Completed running')) { - break; - } - if (data.startsWith('Failed running')) { - writeFileSync(watchedFile, 'console.log("test has ran");'); - } - }; + // Break the chunks into lines + for await (const data of createInterface({ input: child.stdout })) { + if (data.startsWith('Completed running')) { + break; + } + if (data.startsWith('Failed running')) { + writeFileSync(watchedFile, 'console.log("test has ran");'); + } + } } finally { child.kill(); } @@ -233,7 +233,7 @@ describe('watch mode', { concurrency: true, timeout: 60_000 }, () => { messages: { restarted: `Restarting ${inspect(file)}`, completed: `Completed running ${inspect(file)}` }, }); }); - + // TODO: Remove skip after https://github.com/nodejs/node/pull/45271 lands it('should not watch when running an missing file', { skip: !supportsRecursive