From 3263ceb21a9cf382e22cda7f5fd59a5834845897 Mon Sep 17 00:00:00 2001 From: Moshe Atlow Date: Sun, 13 Nov 2022 17:13:26 +0200 Subject: [PATCH] watch: watch for missing dependencies PR-URL: https://github.com/nodejs/node/pull/45348 Reviewed-By: Ruy Adorno Reviewed-By: Benjamin Gruenbaum Reviewed-By: Nitzan Uziely --- 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 | 72 ++++++++++++++++++++---- 6 files changed, 84 insertions(+), 22 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..9b894922621e3b 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -9,10 +9,13 @@ 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()`'); +const supportsRecursive = common.isOSX || common.isWindows; + function restart(file) { // To avoid flakiness, we save the file repeatedly until test is done writeFileSync(file, readFileSync(file)); @@ -59,8 +62,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,11 +77,29 @@ function assertRestartedCorrectly({ stdout, messages: { inner, completed, restar assert.deepStrictEqual(lines.slice(-end.length), end); } +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");'); + } + } + } finally { + child.kill(); + } +} + 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 }); @@ -104,16 +125,8 @@ 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 + skip: !supportsRecursive }, async () => { const file = fixtures.path('watch-mode/subdir/non-existing.js'); const watchedFile = fixtures.path('watch-mode/subdir/file.js'); @@ -220,4 +233,39 @@ describe('watch mode', { concurrency: true, timeout: 60_0000 }, () => { 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 + }, 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', { + 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', { + skip: !supportsRecursive + }, 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', { + skip: !supportsRecursive + }, 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 }); + }); });