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

Improve cwd option #803

Merged
merged 1 commit into from
Feb 8, 2024
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions index.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,6 @@ type CommonOptions<IsSync extends boolean = boolean> = {
/**
Path to the Node.js executable to use in child processes.

This can be either an absolute path or a path relative to the `cwd` option.

Requires `preferLocal` to be `true`.

For example, this can be used together with [`get-node`](https://github.com/ehmicky/get-node) to run a specific Node.js version in a child process.
Expand Down Expand Up @@ -412,6 +410,8 @@ type CommonOptions<IsSync extends boolean = boolean> = {
/**
Current working directory of the child process.

This is also used to resolve the `execPath` option when it is a relative path.

@default process.cwd()
*/
readonly cwd?: string | URL;
Expand Down Expand Up @@ -711,7 +711,7 @@ type ExecaCommonReturnValue<IsSync extends boolean = boolean, OptionsType extend
signalDescription?: string;

/**
The `cwd` of the command if provided in the command options. Otherwise it is `process.cwd()`.
The current directory in which the command was run.
*/
cwd: string;

Expand Down
23 changes: 8 additions & 15 deletions index.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
import {Buffer} from 'node:buffer';
import {setMaxListeners} from 'node:events';
import path from 'node:path';
import {basename} from 'node:path';
import childProcess from 'node:child_process';
import process from 'node:process';
import {fileURLToPath} from 'node:url';
import crossSpawn from 'cross-spawn';
import stripFinalNewline from 'strip-final-newline';
import {npmRunPathEnv} from 'npm-run-path';
Expand All @@ -16,6 +15,7 @@ import {getSpawnedResult, makeAllStream} from './lib/stream.js';
import {mergePromise} from './lib/promise.js';
import {joinCommand, getEscapedCommand} from './lib/escape.js';
import {parseCommand} from './lib/command.js';
import {getDefaultCwd, normalizeCwd, safeNormalizeFileUrl, normalizeFileUrl} from './lib/cwd.js';
import {parseTemplates} from './lib/script.js';
import {logCommand, verboseDefault} from './lib/verbose.js';
import {bufferToUint8Array} from './lib/stdio/utils.js';
Expand All @@ -32,17 +32,7 @@ const getEnv = ({env: envOption, extendEnv, preferLocal, localDir, execPath}) =>
return env;
};

const normalizeFileUrl = file => file instanceof URL ? fileURLToPath(file) : file;

const getFilePath = rawFile => {
const fileString = normalizeFileUrl(rawFile);

if (typeof fileString !== 'string') {
throw new TypeError('First argument must be a string or a file URL.');
}

return fileString;
};
const getFilePath = rawFile => safeNormalizeFileUrl(rawFile, 'First argument');

const handleArguments = (rawFile, rawArgs, rawOptions = {}) => {
const filePath = getFilePath(rawFile);
Expand All @@ -56,8 +46,9 @@ const handleArguments = (rawFile, rawArgs, rawOptions = {}) => {
options.shell = normalizeFileUrl(options.shell);
options.env = getEnv(options);
options.forceKillAfterDelay = normalizeForceKillAfterDelay(options.forceKillAfterDelay);
options.cwd = normalizeCwd(options.cwd);

if (process.platform === 'win32' && path.basename(file, '.exe') === 'cmd') {
if (process.platform === 'win32' && basename(file, '.exe') === 'cmd') {
// #116
args.unshift('/q');
}
Expand All @@ -73,7 +64,7 @@ const addDefaultOptions = ({
stripFinalNewline = true,
extendEnv = true,
preferLocal = false,
cwd = process.cwd(),
cwd = getDefaultCwd(),
localDir = cwd,
execPath = process.execPath,
encoding = 'utf8',
Expand Down Expand Up @@ -210,6 +201,7 @@ const handlePromise = async ({spawned, options, stdioStreamsGroups, originalStre
return {
command,
escapedCommand,
cwd: options.cwd,
failed: false,
timedOut: false,
isCanceled: false,
Expand Down Expand Up @@ -277,6 +269,7 @@ export function execaSync(rawFile, rawArgs, rawOptions) {
return {
command,
escapedCommand,
cwd: options.cwd,
failed: false,
timedOut: false,
isCanceled: false,
Expand Down
49 changes: 49 additions & 0 deletions lib/cwd.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
import {statSync} from 'node:fs';
import {resolve} from 'node:path';
import {fileURLToPath} from 'node:url';
import process from 'node:process';

export const getDefaultCwd = () => {
try {
return process.cwd();
} catch (error) {
error.message = `The current directory does not exist.\n${error.message}`;
throw error;
}
};

export const normalizeCwd = cwd => {
const cwdString = safeNormalizeFileUrl(cwd, 'The "cwd" option');
return resolve(cwdString);
};

export const safeNormalizeFileUrl = (file, name) => {
const fileString = normalizeFileUrl(file);

if (typeof fileString !== 'string') {
throw new TypeError(`${name} must be a string or a file URL: ${fileString}.`);
}

return fileString;
};

export const normalizeFileUrl = file => file instanceof URL ? fileURLToPath(file) : file;

export const fixCwdError = (originalMessage, cwd) => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We cannot perform this cwd validation check before process spawning because it involves making a stat I/O call. Right now, execa() spawns processes right away, and introducing a sync I/O call before spawning for every execa() call is not worth it, just to improve the cwd validation error message.

That being said, since process.cwd() is synchronous, when the cwd option default value is used, it is validated before process spawning.

if (cwd === getDefaultCwd()) {
return originalMessage;
}

let cwdStat;
try {
cwdStat = statSync(cwd);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This function is used both by execa() and execaSync().
In principle, we could use two separate methods, one async and one sync. This would require having two versions of makeError(), one async and one sync.
However, this function happens only when both:

  • the child process failed
  • the cwd option was set with a different value than process.cwd()

So I thought it might be ok to keep it simple for the time being.

Copy link
Owner

Choose a reason for hiding this comment

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

So I thought it might be ok to keep it simple for the time being.

👍

} catch (error) {
return `The "cwd" option is invalid: ${cwd}.\n${error.message}\n${originalMessage}`;
}

if (!cwdStat.isDirectory()) {
return `The "cwd" option is not a directory: ${cwd}.\n${originalMessage}`;
}

return originalMessage;
};
7 changes: 4 additions & 3 deletions lib/error.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import process from 'node:process';
import {signalsByName} from 'human-signals';
import stripFinalNewline from 'strip-final-newline';
import {isBinary, binaryToString} from './stdio/utils.js';
import {fixCwdError} from './cwd.js';

const getErrorPrefix = ({timedOut, timeout, errorCode, signal, signalDescription, exitCode, isCanceled}) => {
if (timedOut) {
Expand Down Expand Up @@ -53,7 +53,7 @@ export const makeError = ({
escapedCommand,
timedOut,
isCanceled,
options: {timeoutDuration: timeout, cwd = process.cwd()},
options: {timeoutDuration: timeout, cwd},
}) => {
// `signal` and `exitCode` emitted on `spawned.on('exit')` event can be `null`.
// We normalize them to `undefined`
Expand All @@ -64,7 +64,8 @@ export const makeError = ({
const errorCode = error?.code;
const prefix = getErrorPrefix({timedOut, timeout, errorCode, signal, signalDescription, exitCode, isCanceled});
const execaMessage = `Command ${prefix}: ${command}`;
const originalMessage = previousErrors.has(error) ? error.originalMessage : String(error?.message ?? error);
const originalErrorMessage = previousErrors.has(error) ? error.originalMessage : String(error?.message ?? error);
const originalMessage = fixCwdError(originalErrorMessage, cwd);
const shortMessage = error === undefined ? execaMessage : `${execaMessage}\n${originalMessage}`;
const messageStdio = all === undefined ? [stdio[2], stdio[1]] : [all];
const message = [shortMessage, ...messageStdio, ...stdio.slice(3)]
Expand Down
6 changes: 3 additions & 3 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -366,7 +366,7 @@ Since the escaping is fairly basic, this should not be executed directly as a pr

Type: `string`

The `cwd` of the command if provided in the [command options](#cwd-1). Otherwise it is `process.cwd()`.
The [current directory](#cwd-1) in which the command was run.

#### stdout

Expand Down Expand Up @@ -510,6 +510,8 @@ Default: `process.cwd()`

Current working directory of the child process.

This is also used to resolve the [`execPath`](#execpath) option when it is a relative path.

#### env

Type: `object`\
Expand Down Expand Up @@ -549,8 +551,6 @@ Default: `process.execPath` (Current Node.js executable)

Path to the Node.js executable to use in child processes.

This can be either an absolute path or a path relative to the [`cwd` option](#cwd).

Requires [`preferLocal`](#preferlocal) to be `true`.

For example, this can be used together with [`get-node`](https://github.com/ehmicky/get-node) to run a specific Node.js version in a child process.
Expand Down
103 changes: 103 additions & 0 deletions test/cwd.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,103 @@
import {mkdir, rmdir} from 'node:fs/promises';
import {relative, toNamespacedPath} from 'node:path';
import process from 'node:process';
import {pathToFileURL, fileURLToPath} from 'node:url';
import tempfile from 'tempfile';
import test from 'ava';
import {execa, execaSync} from '../index.js';
import {FIXTURES_DIR, setFixtureDir} from './helpers/fixtures-dir.js';

setFixtureDir();

const isWindows = process.platform === 'win32';

const testOptionCwdString = async (t, execaMethod) => {
const cwd = '/';
const {stdout} = await execaMethod('node', ['-p', 'process.cwd()'], {cwd});
t.is(toNamespacedPath(stdout), toNamespacedPath(cwd));
};

test('The "cwd" option can be a string', testOptionCwdString, execa);
test('The "cwd" option can be a string - sync', testOptionCwdString, execaSync);

const testOptionCwdUrl = async (t, execaMethod) => {
const cwd = '/';
const cwdUrl = pathToFileURL(cwd);
const {stdout} = await execaMethod('node', ['-p', 'process.cwd()'], {cwd: cwdUrl});
t.is(toNamespacedPath(stdout), toNamespacedPath(cwd));
};

test('The "cwd" option can be a URL', testOptionCwdUrl, execa);
test('The "cwd" option can be a URL - sync', testOptionCwdUrl, execaSync);

const testOptionCwdInvalid = (t, execaMethod) => {
t.throws(() => {
execaMethod('empty.js', {cwd: true});
}, {message: /The "cwd" option must be a string or a file URL: true/});
};

test('The "cwd" option cannot be an invalid type', testOptionCwdInvalid, execa);
test('The "cwd" option cannot be an invalid type - sync', testOptionCwdInvalid, execaSync);

const testErrorCwdDefault = async (t, execaMethod) => {
const {cwd} = await execaMethod('empty.js');
t.is(cwd, process.cwd());
};

test('The "cwd" option defaults to process.cwd()', testErrorCwdDefault, execa);
test('The "cwd" option defaults to process.cwd() - sync', testErrorCwdDefault, execaSync);

// Windows does not allow removing a directory used as `cwd` of a running process
if (!isWindows) {
const testCwdPreSpawn = async (t, execaMethod) => {
const currentCwd = process.cwd();
const filePath = tempfile();
await mkdir(filePath);
process.chdir(filePath);
await rmdir(filePath);

try {
t.throws(() => {
execaMethod('empty.js');
}, {message: /The current directory does not exist/});
} finally {
process.chdir(currentCwd);
}
};

test.serial('The "cwd" option default fails if current cwd is missing', testCwdPreSpawn, execa);
test.serial('The "cwd" option default fails if current cwd is missing - sync', testCwdPreSpawn, execaSync);
}

const cwdNotExisting = {cwd: 'does_not_exist', expectedCode: 'ENOENT', expectedMessage: 'The "cwd" option is invalid'};
const cwdTooLong = {cwd: '.'.repeat(1e5), expectedCode: 'ENAMETOOLONG', expectedMessage: 'The "cwd" option is invalid'};
const cwdNotDir = {cwd: fileURLToPath(import.meta.url), expectedCode: isWindows ? 'ENOENT' : 'ENOTDIR', expectedMessage: 'The "cwd" option is not a directory'};

const testCwdPostSpawn = async (t, {cwd, expectedCode, expectedMessage}, execaMethod) => {
const {failed, code, message} = await execaMethod('empty.js', {cwd, reject: false});
t.true(failed);
t.is(code, expectedCode);
t.true(message.includes(expectedMessage));
t.true(message.includes(cwd));
};

test('The "cwd" option must be an existing file', testCwdPostSpawn, cwdNotExisting, execa);
test('The "cwd" option must be an existing file - sync', testCwdPostSpawn, cwdNotExisting, execaSync);
test('The "cwd" option must not be too long', testCwdPostSpawn, cwdTooLong, execa);
test('The "cwd" option must not be too long - sync', testCwdPostSpawn, cwdTooLong, execaSync);
test('The "cwd" option must be a directory', testCwdPostSpawn, cwdNotDir, execa);
test('The "cwd" option must be a directory - sync', testCwdPostSpawn, cwdNotDir, execaSync);

const successProperties = {fixtureName: 'empty.js', expectedFailed: false};
const errorProperties = {fixtureName: 'fail.js', expectedFailed: true};

const testErrorCwd = async (t, execaMethod, {fixtureName, expectedFailed}) => {
const {failed, cwd} = await execaMethod(fixtureName, {cwd: relative('.', FIXTURES_DIR), reject: false});
t.is(failed, expectedFailed);
t.is(cwd, FIXTURES_DIR);
};

test('result.cwd is defined', testErrorCwd, execa, successProperties);
test('result.cwd is defined - sync', testErrorCwd, execaSync, successProperties);
test('error.cwd is defined', testErrorCwd, execa, errorProperties);
test('error.cwd is defined - sync', testErrorCwd, execaSync, errorProperties);
22 changes: 6 additions & 16 deletions test/error.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import process from 'node:process';
import test from 'ava';
import {execa, execaSync} from '../index.js';
import {FIXTURES_DIR, setFixtureDir} from './helpers/fixtures-dir.js';
import {setFixtureDir} from './helpers/fixtures-dir.js';
import {fullStdio, getStdio} from './helpers/stdio.js';
import {noopGenerator, outputObjectGenerator} from './helpers/generator.js';
import {foobarString} from './helpers/input.js';
Expand All @@ -17,6 +17,7 @@ test('Return value properties are not missing and are ordered', async t => {
t.deepEqual(Reflect.ownKeys(result), [
'command',
'escapedCommand',
'cwd',
'failed',
'timedOut',
'isCanceled',
Expand Down Expand Up @@ -185,8 +186,8 @@ test('error.message newlines are consistent - no newline', testErrorMessageConsi
test('error.message newlines are consistent - newline', testErrorMessageConsistent, 'stdout\n');

test('Original error.message is kept', async t => {
const {originalMessage} = await t.throwsAsync(execa('noop.js', {cwd: 1}));
t.true(originalMessage.startsWith('The "options.cwd" property must be of type string or an instance of Buffer or URL. Received type number'));
const {originalMessage} = await t.throwsAsync(execa('noop.js', {uid: true}));
t.is(originalMessage, 'The "options.uid" property must be int32. Received type boolean (true)');
});

test('failed is false on success', async t => {
Expand Down Expand Up @@ -320,21 +321,10 @@ test('error.code is undefined on success', async t => {
});

test('error.code is defined on failure if applicable', async t => {
const {code} = await t.throwsAsync(execa('noop.js', {cwd: 1}));
const {code} = await t.throwsAsync(execa('noop.js', {uid: true}));
t.is(code, 'ERR_INVALID_ARG_TYPE');
});

test('error.cwd is defined on failure if applicable', async t => {
const {cwd} = await t.throwsAsync(execa('fail.js', [], {cwd: FIXTURES_DIR}));
t.is(cwd, FIXTURES_DIR);
});

test('error.cwd is undefined on failure if not passed as options', async t => {
const expectedCwd = process.cwd();
const {cwd} = await t.throwsAsync(execa('fail.js'));
t.is(cwd, expectedCwd);
});

const testUnusualError = async (t, error) => {
const childProcess = execa('empty.js');
childProcess.emit('error', error);
Expand Down Expand Up @@ -367,7 +357,7 @@ test('error instance can be a plain object', async t => {
test('error instance can be shared', async t => {
const originalMessage = foobarString;
const error = new Error(originalMessage);
const fixtureName = 'noop.js';
const fixtureName = 'empty.js';

const firstArgument = 'one';
const childProcess = execa(fixtureName, [firstArgument]);
Expand Down
6 changes: 3 additions & 3 deletions test/helpers/fixtures-dir.js
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
import path from 'node:path';
import {delimiter, resolve} from 'node:path';
import process from 'node:process';
import {fileURLToPath} from 'node:url';
import pathKey from 'path-key';

export const PATH_KEY = pathKey();
export const FIXTURES_DIR_URL = new URL('../fixtures/', import.meta.url);
export const FIXTURES_DIR = fileURLToPath(FIXTURES_DIR_URL);
export const FIXTURES_DIR = resolve(fileURLToPath(FIXTURES_DIR_URL));

// Add the fixtures directory to PATH so fixtures can be executed without adding
// `node`. This is only meant to make writing tests simpler.
export const setFixtureDir = () => {
process.env[PATH_KEY] = FIXTURES_DIR + path.delimiter + process.env[PATH_KEY];
process.env[PATH_KEY] = FIXTURES_DIR + delimiter + process.env[PATH_KEY];
};

Loading
Loading