Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

watch: watch for unexsiting dependencies #45348

Merged
merged 5 commits into from Nov 13, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 9 additions & 1 deletion lib/internal/modules/cjs/loader.js
Expand Up @@ -28,6 +28,7 @@ const {
ArrayPrototypeIncludes,
ArrayPrototypeIndexOf,
ArrayPrototypeJoin,
ArrayPrototypeMap,
ArrayPrototypePush,
ArrayPrototypeSlice,
ArrayPrototypeSplice,
Expand Down Expand Up @@ -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}`)) });
}
}

Expand Down Expand Up @@ -648,6 +655,7 @@ Module._findPath = function(request, paths, isMain) {
Module._pathCache[cacheKey] = filename;
return filename;
}
reportModuleNotFoundToWatchMode(basePath, ArrayPrototypeConcat([''], exts));
}

return false;
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/modules/esm/loader.js
Expand Up @@ -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(
Expand Down
3 changes: 3 additions & 0 deletions lib/internal/modules/esm/resolve.js
Expand Up @@ -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) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we prefix all our environment variables with NODE_: https://nodejs.org/api/cli.html#environment-variables

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will fix in a follow up PR, as this environment variable existed before this pr

process.send({ 'watch:require': [path || resolved.pathname] });
}
throw new ERR_MODULE_NOT_FOUND(
path || resolved.pathname, base && fileURLToPath(base), 'module');
}
Expand Down
11 changes: 7 additions & 4 deletions lib/internal/watch_mode/files_watcher.js
@@ -1,6 +1,8 @@
'use strict';

const {
ArrayIsArray,
ArrayPrototypeForEach,
SafeMap,
SafeSet,
StringPrototypeStartsWith,
Expand Down Expand Up @@ -94,6 +96,7 @@ class FilesWatcher extends EventEmitter {
}

filterFile(file) {
if (!file) return;
if (supportsRecursiveWatching) {
this.watchPath(dirname(file));
} else {
Expand All @@ -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
Expand Down
8 changes: 4 additions & 4 deletions test/fixtures/watch-mode/ipc.js
Expand Up @@ -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()] });
72 changes: 60 additions & 12 deletions test/sequential/test-watch-mode.mjs
Expand Up @@ -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));
Expand Down Expand Up @@ -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;
}
Expand All @@ -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 }, () => {
MoLow marked this conversation as resolved.
Show resolved Hide resolved
it('should watch changes to a file - event loop ended', async () => {
const file = createTmpFile();
const { stderr, stdout } = await spawnWithRestarts({ file });
Expand All @@ -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`);
});

MoLow marked this conversation as resolved.
Show resolved Hide resolved
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');
Expand Down Expand Up @@ -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 });
});
});