From 5a4e308a22a7cf2cb8c93ce62917340728dac1f6 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Wed, 3 Apr 2024 16:56:20 +0100 Subject: [PATCH 1/4] watch: fix some node argument not passed to watched process --- lib/internal/main/watch_mode.js | 38 ++++++- test/sequential/test-watch-mode.mjs | 149 +++++++++++++++++++++++++++- 2 files changed, 179 insertions(+), 8 deletions(-) diff --git a/lib/internal/main/watch_mode.js b/lib/internal/main/watch_mode.js index ab9f9fa95bc271..9289669866058b 100644 --- a/lib/internal/main/watch_mode.js +++ b/lib/internal/main/watch_mode.js @@ -37,10 +37,40 @@ const kWatchedPaths = ArrayPrototypeMap(getOptionValue('--watch-path'), (path) = const kPreserveOutput = getOptionValue('--watch-preserve-output'); const kCommand = ArrayPrototypeSlice(process.argv, 1); const kCommandStr = inspect(ArrayPrototypeJoin(kCommand, ' ')); -const args = ArrayPrototypeFilter(process.execArgv, (arg, i, arr) => - !StringPrototypeStartsWith(arg, '--watch-path') && - (!arr[i - 1] || !StringPrototypeStartsWith(arr[i - 1], '--watch-path')) && - arg !== '--watch' && !StringPrototypeStartsWith(arg, '--watch=') && arg !== '--watch-preserve-output'); + +const watchOptions = ['--watch', '--watch-path', '--watch-preserve-output']; + +function argIsWatchOptionWithValue(arg) { + return watchOptions.some((watchOption) => StringPrototypeStartsWith(arg, `${watchOption}=`)); +} + +function argIsWatchOption(arg) { + return watchOptions.includes(arg) || argIsWatchOptionWithValue(arg); +} + +const args = ArrayPrototypeFilter(process.execArgv, (arg, i, arr) => { + // Don't pass --watch related args to children + if (argIsWatchOption(arg)) { + return false; + } + + // First arg so it cant be a value of some watch option + if (i === 0) { + return true; + } + + const prevArg = arr[i - 1]; + const prevArgIsWatchOption = argIsWatchOption(prevArg); + + // This is not a value of arg option as previous arg is also not watch related + if (!prevArgIsWatchOption) { + return true; + } + + // If prev arg is watch related, but it has a value it means than this arg is not a value of such option + return argIsWatchOptionWithValue(prevArg); +}); + ArrayPrototypePushApply(args, kCommand); const watcher = new FilesWatcher({ debounce: 200, mode: kShouldFilterModules ? 'filter' : 'all' }); diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index cbfde306d6c963..03e7b8d5f940c2 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -30,7 +30,14 @@ function createTmpFile(content = 'console.log("running");', ext = '.js', basenam } async function runWriteSucceed({ - file, watchedFile, watchFlag = '--watch', args = [file], completed = 'Completed running', restarts = 2, options = {} + file, + watchedFile, + watchFlag = '--watch', + args = [file], + completed = 'Completed running', + restarts = 2, + options = {}, + shouldFail = false }) { const child = spawn(execPath, [watchFlag, '--no-warnings', ...args], { encoding: 'utf8', stdio: 'pipe', ...options }); let completes = 0; @@ -57,6 +64,10 @@ async function runWriteSucceed({ cancelRestarts = restart(watchedFile); } } + + if (!shouldFail && data.startsWith('Failed running')) { + break; + } } } finally { child.kill(); @@ -120,7 +131,12 @@ describe('watch mode', { concurrency: !process.env.TEST_PARALLEL, timeout: 60_00 it('should watch changes to a failing file', async () => { const file = createTmpFile('throw new Error("fails");'); - const { stderr, stdout } = await runWriteSucceed({ file, watchedFile: file, completed: 'Failed running' }); + const { stderr, stdout } = await runWriteSucceed({ + file, + watchedFile: file, + completed: 'Failed running', + shouldFail: true + }); assert.match(stderr, /Error: fails\r?\n/); assert.deepStrictEqual(stdout, [ @@ -159,7 +175,13 @@ describe('watch mode', { concurrency: !process.env.TEST_PARALLEL, timeout: 60_00 const file = path.join(dir, 'non-existing.js'); const watchedFile = createTmpFile('', '.js', dir); const args = ['--watch-path', dir, file]; - const { stderr, stdout } = await runWriteSucceed({ file, watchedFile, args, completed: 'Failed running' }); + const { stderr, stdout } = await runWriteSucceed({ + file, + watchedFile, + args, + completed: 'Failed running', + shouldFail: true + }); assert.match(stderr, /Error: Cannot find module/g); assert.deepStrictEqual(stdout, [ @@ -177,7 +199,13 @@ describe('watch mode', { concurrency: !process.env.TEST_PARALLEL, timeout: 60_00 const file = path.join(dir, 'non-existing.js'); const watchedFile = createTmpFile('', '.js', dir); const args = [`--watch-path=${dir}`, file]; - const { stderr, stdout } = await runWriteSucceed({ file, watchedFile, args, completed: 'Failed running' }); + const { stderr, stdout } = await runWriteSucceed({ + file, + watchedFile, + args, + completed: 'Failed running', + shouldFail: true + }); assert.match(stderr, /Error: Cannot find module/g); assert.deepStrictEqual(stdout, [ @@ -372,4 +400,117 @@ console.log(values.random); `Completed running ${inspect(file)}`, ]); }); + + it('when --watch-path has an equals and --require does not and --require is after the --watch-path and require is' + + 'requiring from node modules package exports, required path should run', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project1'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + // Create a package example + const demoPackageDir = path.join(projectDir, 'node_modules/demo'); + mkdirSync(demoPackageDir, { recursive: true }); + + writeFileSync(path.join(demoPackageDir, 'package.json'), JSON.stringify({ + 'name': 'demo', + 'version': '1.0.0', + 'main': 'some.js', + 'exports': { + './some': './some.js' + } + } + )); + + writeFileSync(path.join(demoPackageDir, 'some.js'), 'console.log("hello")'); + + const file = createTmpFile(undefined, undefined, projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = [`--watch-path=${dir}`, '--require', 'demo/some', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); + + it('when --watch-path has an equals and --require does not and --require is after the --watch-path,' + + 'the required file and the main module file should ran', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project2'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + writeFileSync(path.join(projectDir, 'some.js'), 'console.log(\'hello\')'); + + const file = createTmpFile('console.log(\'running\');', '.js', projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = [`--watch-path=${dir}`, '--require', './some.js', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); + + it('when --watch-path and --require has an equals and --require is after the --watch-path,' + + 'the required file and the main module file should ran', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project3'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + writeFileSync(path.join(projectDir, 'some.js'), "console.log('hello')"); + + const file = createTmpFile("console.log('running');", '.js', projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = [`--watch-path=${dir}`, '--require=./some.js', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); }); From 8bba166da2f13b2f312d06a1d8a3249da3986bd2 Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 4 Apr 2024 10:14:02 +0100 Subject: [PATCH 2/4] test: add tests for without equals --- test/sequential/test-watch-mode.mjs | 113 ++++++++++++++++++++++++++++ 1 file changed, 113 insertions(+) diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index 03e7b8d5f940c2..d37d474a0d0858 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -513,4 +513,117 @@ console.log(values.random); `Completed running ${inspect(file)}`, ]); }); + + it('when --watch-path and --require does not have an equals and --require is after the --watch-path and require is' + + 'requiring from node modules package exports, required path should run', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project4'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + // Create a package example + const demoPackageDir = path.join(projectDir, 'node_modules/demo'); + mkdirSync(demoPackageDir, { recursive: true }); + + writeFileSync(path.join(demoPackageDir, 'package.json'), JSON.stringify({ + 'name': 'demo', + 'version': '1.0.0', + 'main': 'some.js', + 'exports': { + './some': './some.js' + } + } + )); + + writeFileSync(path.join(demoPackageDir, 'some.js'), 'console.log("hello")'); + + const file = createTmpFile(undefined, undefined, projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = ['--watch-path', `${dir}`, '--require', 'demo/some', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); + + it('when --watch-path and --require does not have an equals and --require is after the --watch-path,' + + 'the required file and the main module file should ran', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project5'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + writeFileSync(path.join(projectDir, 'some.js'), 'console.log(\'hello\')'); + + const file = createTmpFile('console.log(\'running\');', '.js', projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = ['--watch-path', `${dir}`, '--require', './some.js', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); + + it('when --watch-path does not have an equal and --require does and --require is after the --watch-path,' + + 'the required file and the main module file should ran', { + skip: !supportsRecursive, + }, async () => { + const projectDir = tmpdir.resolve('project6'); + mkdirSync(projectDir); + + const dir = path.join(projectDir, 'watched-dir'); + mkdirSync(dir); + + writeFileSync(path.join(projectDir, 'some.js'), "console.log('hello')"); + + const file = createTmpFile("console.log('running');", '.js', projectDir); + const watchedFile = createTmpFile('', '.js', dir); + const args = ['--watch-path', `${dir}`, '--require=./some.js', file]; + const { stdout, stderr } = await runWriteSucceed({ + file, watchedFile, args, options: { + cwd: projectDir + } + }); + + assert.strictEqual(stderr, ''); + assert.deepStrictEqual(stdout, [ + 'hello', + 'running', + `Completed running ${inspect(file)}`, + `Restarting ${inspect(file)}`, + 'hello', + 'running', + `Completed running ${inspect(file)}`, + ]); + }); }); From 57756e8e5b5020af726cbaa245aee9debf1a77ea Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 4 Apr 2024 10:39:12 +0100 Subject: [PATCH 3/4] test: remove node module test as it covert in other places + simplify test --- lib/internal/main/watch_mode.js | 46 ++++-------- test/sequential/test-watch-mode.mjs | 106 ++-------------------------- 2 files changed, 16 insertions(+), 136 deletions(-) diff --git a/lib/internal/main/watch_mode.js b/lib/internal/main/watch_mode.js index 9289669866058b..91610c0573fccc 100644 --- a/lib/internal/main/watch_mode.js +++ b/lib/internal/main/watch_mode.js @@ -1,12 +1,10 @@ 'use strict'; const { - ArrayPrototypeFilter, ArrayPrototypeForEach, ArrayPrototypeJoin, ArrayPrototypeMap, ArrayPrototypePushApply, ArrayPrototypeSlice, - StringPrototypeStartsWith, } = primordials; const { @@ -38,40 +36,20 @@ const kPreserveOutput = getOptionValue('--watch-preserve-output'); const kCommand = ArrayPrototypeSlice(process.argv, 1); const kCommandStr = inspect(ArrayPrototypeJoin(kCommand, ' ')); -const watchOptions = ['--watch', '--watch-path', '--watch-preserve-output']; +const argsWithoutWatchOptions = []; -function argIsWatchOptionWithValue(arg) { - return watchOptions.some((watchOption) => StringPrototypeStartsWith(arg, `${watchOption}=`)); -} - -function argIsWatchOption(arg) { - return watchOptions.includes(arg) || argIsWatchOptionWithValue(arg); -} - -const args = ArrayPrototypeFilter(process.execArgv, (arg, i, arr) => { - // Don't pass --watch related args to children - if (argIsWatchOption(arg)) { - return false; - } - - // First arg so it cant be a value of some watch option - if (i === 0) { - return true; - } - - const prevArg = arr[i - 1]; - const prevArgIsWatchOption = argIsWatchOption(prevArg); - - // This is not a value of arg option as previous arg is also not watch related - if (!prevArgIsWatchOption) { - return true; +for (let i = 0; i < process.execArgv.length; i++) { + const arg = process.execArgv[i]; + if (arg.startsWith('--watch')) { + if(!arg.includes('=')) { + i++; + } + continue; } + argsWithoutWatchOptions.push(arg); +} - // If prev arg is watch related, but it has a value it means than this arg is not a value of such option - return argIsWatchOptionWithValue(prevArg); -}); - -ArrayPrototypePushApply(args, kCommand); +ArrayPrototypePushApply(argsWithoutWatchOptions, kCommand); const watcher = new FilesWatcher({ debounce: 200, mode: kShouldFilterModules ? 'filter' : 'all' }); ArrayPrototypeForEach(kWatchedPaths, (p) => watcher.watchPath(p)); @@ -83,7 +61,7 @@ let exited; function start() { exited = false; const stdio = kShouldFilterModules ? ['inherit', 'inherit', 'inherit', 'ipc'] : 'inherit'; - child = spawn(process.execPath, args, { stdio, env: { ...process.env, WATCH_REPORT_DEPENDENCIES: '1' } }); + child = spawn(process.execPath, argsWithoutWatchOptions, { stdio, env: { ...process.env, WATCH_REPORT_DEPENDENCIES: '1' } }); watcher.watchChildProcessModules(child); child.once('exit', (code) => { exited = true; diff --git a/test/sequential/test-watch-mode.mjs b/test/sequential/test-watch-mode.mjs index d37d474a0d0858..4f029602a11b6b 100644 --- a/test/sequential/test-watch-mode.mjs +++ b/test/sequential/test-watch-mode.mjs @@ -401,55 +401,7 @@ console.log(values.random); ]); }); - it('when --watch-path has an equals and --require does not and --require is after the --watch-path and require is' + - 'requiring from node modules package exports, required path should run', { - skip: !supportsRecursive, - }, async () => { - const projectDir = tmpdir.resolve('project1'); - mkdirSync(projectDir); - - const dir = path.join(projectDir, 'watched-dir'); - mkdirSync(dir); - - // Create a package example - const demoPackageDir = path.join(projectDir, 'node_modules/demo'); - mkdirSync(demoPackageDir, { recursive: true }); - - writeFileSync(path.join(demoPackageDir, 'package.json'), JSON.stringify({ - 'name': 'demo', - 'version': '1.0.0', - 'main': 'some.js', - 'exports': { - './some': './some.js' - } - } - )); - - writeFileSync(path.join(demoPackageDir, 'some.js'), 'console.log("hello")'); - - const file = createTmpFile(undefined, undefined, projectDir); - const watchedFile = createTmpFile('', '.js', dir); - const args = [`--watch-path=${dir}`, '--require', 'demo/some', file]; - const { stdout, stderr } = await runWriteSucceed({ - file, watchedFile, args, options: { - cwd: projectDir - } - }); - - assert.strictEqual(stderr, ''); - assert.deepStrictEqual(stdout, [ - 'hello', - 'running', - `Completed running ${inspect(file)}`, - `Restarting ${inspect(file)}`, - 'hello', - 'running', - `Completed running ${inspect(file)}`, - ]); - }); - - it('when --watch-path has an equals and --require does not and --require is after the --watch-path,' + - 'the required file and the main module file should ran', { + it('should run when `--watch-path=./foo --require ./bar.js`', { skip: !supportsRecursive, }, async () => { const projectDir = tmpdir.resolve('project2'); @@ -481,8 +433,7 @@ console.log(values.random); ]); }); - it('when --watch-path and --require has an equals and --require is after the --watch-path,' + - 'the required file and the main module file should ran', { + it('should run when `--watch-path=./foo --require=./bar.js`', { skip: !supportsRecursive, }, async () => { const projectDir = tmpdir.resolve('project3'); @@ -514,55 +465,7 @@ console.log(values.random); ]); }); - it('when --watch-path and --require does not have an equals and --require is after the --watch-path and require is' + - 'requiring from node modules package exports, required path should run', { - skip: !supportsRecursive, - }, async () => { - const projectDir = tmpdir.resolve('project4'); - mkdirSync(projectDir); - - const dir = path.join(projectDir, 'watched-dir'); - mkdirSync(dir); - - // Create a package example - const demoPackageDir = path.join(projectDir, 'node_modules/demo'); - mkdirSync(demoPackageDir, { recursive: true }); - - writeFileSync(path.join(demoPackageDir, 'package.json'), JSON.stringify({ - 'name': 'demo', - 'version': '1.0.0', - 'main': 'some.js', - 'exports': { - './some': './some.js' - } - } - )); - - writeFileSync(path.join(demoPackageDir, 'some.js'), 'console.log("hello")'); - - const file = createTmpFile(undefined, undefined, projectDir); - const watchedFile = createTmpFile('', '.js', dir); - const args = ['--watch-path', `${dir}`, '--require', 'demo/some', file]; - const { stdout, stderr } = await runWriteSucceed({ - file, watchedFile, args, options: { - cwd: projectDir - } - }); - - assert.strictEqual(stderr, ''); - assert.deepStrictEqual(stdout, [ - 'hello', - 'running', - `Completed running ${inspect(file)}`, - `Restarting ${inspect(file)}`, - 'hello', - 'running', - `Completed running ${inspect(file)}`, - ]); - }); - - it('when --watch-path and --require does not have an equals and --require is after the --watch-path,' + - 'the required file and the main module file should ran', { + it('should run when `--watch-path ./foo --require ./bar.js`', { skip: !supportsRecursive, }, async () => { const projectDir = tmpdir.resolve('project5'); @@ -594,8 +497,7 @@ console.log(values.random); ]); }); - it('when --watch-path does not have an equal and --require does and --require is after the --watch-path,' + - 'the required file and the main module file should ran', { + it('should run when `--watch-path=./foo --require=./bar.js`', { skip: !supportsRecursive, }, async () => { const projectDir = tmpdir.resolve('project6'); From dbf624602df96417febd8a252a0687ce86a57ced Mon Sep 17 00:00:00 2001 From: Raz Luvaton <16746759+rluvaton@users.noreply.github.com> Date: Thu, 4 Apr 2024 11:09:13 +0100 Subject: [PATCH 4/4] watch: use primordials and fix lint --- lib/internal/main/watch_mode.js | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/internal/main/watch_mode.js b/lib/internal/main/watch_mode.js index 91610c0573fccc..c594f64bfca87d 100644 --- a/lib/internal/main/watch_mode.js +++ b/lib/internal/main/watch_mode.js @@ -3,8 +3,11 @@ const { ArrayPrototypeForEach, ArrayPrototypeJoin, ArrayPrototypeMap, + ArrayPrototypePush, ArrayPrototypePushApply, ArrayPrototypeSlice, + StringPrototypeIncludes, + StringPrototypeStartsWith, } = primordials; const { @@ -40,13 +43,13 @@ const argsWithoutWatchOptions = []; for (let i = 0; i < process.execArgv.length; i++) { const arg = process.execArgv[i]; - if (arg.startsWith('--watch')) { - if(!arg.includes('=')) { + if (StringPrototypeStartsWith(arg, '--watch')) { + if (!StringPrototypeIncludes(arg, '=')) { i++; } continue; } - argsWithoutWatchOptions.push(arg); + ArrayPrototypePush(argsWithoutWatchOptions, arg); } ArrayPrototypePushApply(argsWithoutWatchOptions, kCommand); @@ -61,7 +64,13 @@ let exited; function start() { exited = false; const stdio = kShouldFilterModules ? ['inherit', 'inherit', 'inherit', 'ipc'] : 'inherit'; - child = spawn(process.execPath, argsWithoutWatchOptions, { stdio, env: { ...process.env, WATCH_REPORT_DEPENDENCIES: '1' } }); + child = spawn(process.execPath, argsWithoutWatchOptions, { + stdio, + env: { + ...process.env, + WATCH_REPORT_DEPENDENCIES: '1', + }, + }); watcher.watchChildProcessModules(child); child.once('exit', (code) => { exited = true;