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

Consistently perform bootstrap and encode Brotli config for improved caching and reduced code complexity #1836

Closed
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
245 changes: 162 additions & 83 deletions src/bin.ts

Large diffs are not rendered by default.

29 changes: 11 additions & 18 deletions src/child/child-entrypoint.ts
Original file line number Diff line number Diff line change
@@ -1,24 +1,17 @@
import { BootstrapState, bootstrap } from '../bin';
import { argPrefix, compress, decompress } from './argv-payload';
import {
completeBootstrap,
BootstrapStateInitialProcess,
BootstrapStateForForkedProcesses,
} from '../bin';
import { argPrefix, decompress } from './argv-payload';

const base64ConfigArg = process.argv[2];
if (!base64ConfigArg.startsWith(argPrefix)) throw new Error('unexpected argv');
const base64Payload = base64ConfigArg.slice(argPrefix.length);
const state = decompress(base64Payload) as BootstrapState;
const state = decompress(base64Payload) as
| BootstrapStateForForkedProcesses
| BootstrapStateInitialProcess;

state.isInChildProcess = true;
state.tsNodeScript = __filename;
state.parseArgvResult.argv = process.argv;
state.parseArgvResult.restArgs = process.argv.slice(3);
state.restArgs = process.argv.slice(3);

// Modify and re-compress the payload delivered to subsequent child processes.
// This logic may be refactored into bin.ts by https://github.com/TypeStrong/ts-node/issues/1831
if (state.isCli) {
const stateForChildren: BootstrapState = {
...state,
isCli: false,
};
state.parseArgvResult.argv[2] = `${argPrefix}${compress(stateForChildren)}`;
}

bootstrap(state);
completeBootstrap(state);
36 changes: 36 additions & 0 deletions src/child/child-exec-args.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
import { pathToFileURL } from 'url';
import { brotliCompressSync } from 'zlib';
import type {
BootstrapStateForForkedProcesses,
BootstrapStateInitialProcess,
} from '../bin';
import { argPrefix } from './argv-payload';

export function getChildProcessArguments(
enableEsmLoader: boolean,
state: BootstrapStateForForkedProcesses | BootstrapStateInitialProcess
) {
const nodeExecArgs = [];

if (enableEsmLoader) {
nodeExecArgs.push(
'--require',
require.resolve('./child-require.js'),
'--loader',
// Node on Windows doesn't like `c:\` absolute paths here; must be `file:///c:/`
pathToFileURL(require.resolve('../../child-loader.mjs')).toString()
);
}

const childScriptArgs = [
`${argPrefix}${brotliCompressSync(
Buffer.from(JSON.stringify(state), 'utf8')
).toString('base64')}`,
];

return {
nodeExecArgs,
childScriptArgs,
childScriptPath: require.resolve('./child-entrypoint.js'),
};
}
43 changes: 43 additions & 0 deletions src/child/spawn-child-with-esm.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
import { fork } from 'child_process';
import type { BootstrapStateInitialProcess } from '../bin';
import { getChildProcessArguments } from './child-exec-args';

/**
* @internal
* @param state Bootstrap state to be transferred into the child process.
* @param enableEsmLoader Whether to enable the ESM loader or not. This option may
* be removed in the future when `--esm` is no longer a choice.
* @param targetCwd Working directory to be preserved when transitioning to
* the child process.
*/
export function callInChildWithEsm(
state: BootstrapStateInitialProcess,
targetCwd: string
) {
const { childScriptArgs, childScriptPath, nodeExecArgs } =
getChildProcessArguments(/* enableEsmLoader */ true, state);

childScriptArgs.push(...state.restArgs);

const child = fork(childScriptPath, childScriptArgs, {
stdio: 'inherit',
execArgv: [...process.execArgv, ...nodeExecArgs],
cwd: targetCwd,
});
child.on('error', (error) => {
console.error(error);
process.exit(1);
});
child.on('exit', (code) => {
child.removeAllListeners();
process.off('SIGINT', sendSignalToChild);
process.off('SIGTERM', sendSignalToChild);
process.exitCode = code === null ? 1 : code;
});
// Ignore sigint and sigterm in parent; pass them to child
process.on('SIGINT', sendSignalToChild);
process.on('SIGTERM', sendSignalToChild);
function sendSignalToChild(signal: string) {
process.kill(child.pid, signal);
}
}
46 changes: 0 additions & 46 deletions src/child/spawn-child.ts

This file was deleted.

52 changes: 37 additions & 15 deletions src/test/esm-loader.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -388,10 +388,8 @@ test.suite('esm', (test) => {
test.suite('esm child process working directory', (test) => {
test('should have the correct working directory in the user entry-point', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm/ index.ts`,
{
cwd: resolve(TEST_DIR, 'working-dir'),
}
`${BIN_PATH} --esm index.ts`,
{ cwd: './working-dir/esm/' }
);

expect(err).toBe(null);
Expand All @@ -403,33 +401,57 @@ test.suite('esm', (test) => {
test.suite('esm child process and forking', (test) => {
test('should be able to fork vanilla NodeJS script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-js/index.ts`
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-js-worker/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});

test('should be able to fork TypeScript script', async () => {
test('should be able to fork into a nested TypeScript ESM script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-ts/index.ts`
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-esm-worker/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});

test('should be able to fork TypeScript script by absolute path', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm --cwd ./esm-child-process/ ./process-forking-ts-abs/index.ts`
);
test(
'should be possible to fork into a nested TypeScript script with respect to ' +
'the working directory',
async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm index.ts`,
{ cwd: './esm-child-process/process-forking-subdir-relative/' }
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
}
);

test.suite(
'with NodeNext TypeScript resolution and `.mts` extension',
(test) => {
test.runIf(tsSupportsStableNodeNextNode16);

test('should be able to fork into a nested TypeScript ESM script', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --esm ./esm-child-process/process-forking-esm-worker-next/index.mts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
}
);
});

test.suite('parent passes signals to child', (test) => {
Expand Down
4 changes: 4 additions & 0 deletions src/test/helpers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@ export const BIN_SCRIPT_PATH = join(
TEST_DIR,
'node_modules/.bin/ts-node-script'
);
export const CHILD_ENTRY_POINT_SCRIPT = join(
TEST_DIR,
'node_modules/ts-node/dist/child/child-entrypoint.js'
);
export const BIN_CWD_PATH = join(TEST_DIR, 'node_modules/.bin/ts-node-cwd');
export const BIN_ESM_PATH = join(TEST_DIR, 'node_modules/.bin/ts-node-esm');

Expand Down
73 changes: 49 additions & 24 deletions src/test/index.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { tmpdir } from 'os';
import semver = require('semver');
import {
BIN_PATH_JS,
CHILD_ENTRY_POINT_SCRIPT,
CMD_TS_NODE_WITH_PROJECT_TRANSPILE_ONLY_FLAG,
ts,
tsSupportsMtsCtsExtensions,
Expand Down Expand Up @@ -604,30 +605,27 @@ test.suite('ts-node', (test) => {

test('should have the correct working directory in the user entry-point', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./cjs index.ts`,
{
cwd: resolve(TEST_DIR, 'working-dir'),
}
`${BIN_PATH} --cwd ./working-dir/cjs/ index.ts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing');
expect(stderr).toBe('');
});

// Disabled due to bug:
// --cwd is passed to forked children when not using --esm, erroneously affects their entrypoint resolution.
// tracked/fixed by either https://github.com/TypeStrong/ts-node/issues/1834
// or https://github.com/TypeStrong/ts-node/issues/1831
test.skip('should be able to fork into a nested TypeScript script with a modified working directory', async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./working-dir/forking/ index.ts`
);
test(
'should be able to fork into a nested TypeScript script with a modified ' +
'working directory',
async () => {
const { err, stdout, stderr } = await exec(
`${BIN_PATH} --cwd ./working-dir/forking/ index.ts`
);

expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
});
expect(err).toBe(null);
expect(stdout.trim()).toBe('Passing: from main');
expect(stderr).toBe('');
}
);

test.suite('should read ts-node options from tsconfig.json', (test) => {
const BIN_EXEC = `"${BIN_PATH}" --project tsconfig-options/tsconfig.json`;
Expand Down Expand Up @@ -1086,35 +1084,62 @@ test('Falls back to transpileOnly when ts compiler returns emitSkipped', async (

test.suite('node environment', (test) => {
test.suite('Sets argv and execArgv correctly in forked processes', (test) => {
forkTest(`node --no-warnings ${BIN_PATH_JS}`, BIN_PATH_JS, '--no-warnings');
forkTest(test, `node --no-warnings ${BIN_PATH_JS}`, BIN_PATH_JS, [
'--no-warnings',
]);
forkTest(
test,
`${BIN_PATH}`,
process.platform === 'win32' ? BIN_PATH_JS : BIN_PATH
);

test.suite('with esm enabled', (test) => {
forkTest(
test,
`node --no-warnings ${BIN_PATH_JS} --esm`,
CHILD_ENTRY_POINT_SCRIPT,
[
'--no-warnings',
// Forked child processes should directly receive the necessary ESM loader
// Node flags through `execArgv`, avoiding doubled child process spawns.
'--require',
expect.stringMatching(/child-require.js$/),
'--loader',
expect.stringMatching(/child-loader.mjs$/),
],
'./recursive-fork-esm/index.ts'
);
});

function forkTest(
testFn: typeof test,
command: string,
expectParentArgv0: string,
nodeFlag?: string
nodeFlags?: (string | ReturnType<typeof expect.stringMatching>)[],
testFixturePath = './recursive-fork/index.ts'
) {
test(command, async (t) => {
testFn(command, async (t) => {
const { err, stderr, stdout } = await exec(
`${command} --skipIgnore ./recursive-fork/index.ts argv2`
`${command} --skipIgnore ${testFixturePath} argv2`
);
expect(err).toBeNull();
expect(stderr).toBe('');
const generations = stdout.split('\n');
const expectation = {
execArgv: [nodeFlag, BIN_PATH_JS, '--skipIgnore'].filter((v) => v),
execArgv: [
...(nodeFlags || []),
CHILD_ENTRY_POINT_SCRIPT,
expect.stringMatching(/^--brotli-base64-config=.*/),
],
argv: [
// Note: argv[0] is *always* BIN_PATH_JS in child & grandchild
// Note: argv[0] is *always* CHILD_ENTRY_POINT_SCRIPT in child & grandchild
expectParentArgv0,
resolve(TEST_DIR, 'recursive-fork/index.ts'),
resolve(TEST_DIR, testFixturePath),
'argv2',
],
};
expect(JSON.parse(generations[0])).toMatchObject(expectation);
expectation.argv[0] = BIN_PATH_JS;
expectation.argv[0] = CHILD_ENTRY_POINT_SCRIPT;
expect(JSON.parse(generations[1])).toMatchObject(expectation);
expect(JSON.parse(generations[2])).toMatchObject(expectation);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,10 @@ import { fileURLToPath } from 'url';
// worker process finishes properly with the expected stdout message.
process.exitCode = 1;

const workerProcess = fork(
join(dirname(fileURLToPath(import.meta.url)), 'subfolder/worker.ts'),
[],
{
stdio: 'pipe',
}
);
const containingDir = dirname(fileURLToPath(import.meta.url));
const workerProcess = fork(join(containingDir, 'worker.mts'), [], {
stdio: 'pipe',
});

let stdout = '';

Expand Down
Loading