Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
1 change: 1 addition & 0 deletions tools/egg-bin/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,7 @@
"@eggjs/tegg-vitest": "workspace:*",
"@eggjs/utils": "workspace:*",
"@oclif/core": "catalog:",
"@oxc-node/core": "catalog:",
"@vitest/coverage-v8": "catalog:",
"ci-parallel-vars": "catalog:",
"detect-port": "catalog:",
Expand Down
40 changes: 29 additions & 11 deletions tools/egg-bin/src/baseCommand.ts
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ export abstract class BaseCommand<T extends typeof Command> extends Command {
}),
tscompiler: Flags.string({
helpGroup: 'GLOBAL',
summary: 'TypeScript compiler, like ts-node/register',
summary: 'TypeScript compiler, like @oxc-node/core/register',
aliases: ['tsc'],
}),
// flag with no value (--typescript)
Expand Down Expand Up @@ -225,28 +225,46 @@ export abstract class BaseCommand<T extends typeof Command> extends Command {
throw new Error(`Cannot resolve '${specifier}' from ${findPaths.join(', ')}`);
};
this.isESM = pkg.type === 'module';
// oxc-node's register entry installs BOTH a CJS require hook (via pirates)
// and an ESM `module.register()` hook from a single `--import`, so when it is
// the active compiler an ESM app needs no separate `--loader` (see below).
let isOxcCompiler = false;
if (typescript) {
flags.tscompiler = flags.tscompiler ?? 'ts-node/register';
const tsNodeRegister = cjsResolve(flags.tscompiler);
flags.tscompiler = tsNodeRegister;
// should require tsNodeRegister on current process, let it can require *.ts files
// e.g.: dev command will execute egg loader to find configs and plugins
// await importModule(tsNodeRegister);
// let child process auto require ts-node too
this.addNodeOptions(this.formatImportModule(tsNodeRegister));
flags.tscompiler = flags.tscompiler ?? '@oxc-node/core/register';
isOxcCompiler = flags.tscompiler.includes('@oxc-node/core');

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Avoid using broad substring checks like 'includes()' on file paths or package specifiers to prevent incorrect matches on similarly named directories or files. Instead, match against exact path segments or precise specifier prefixes.

Suggested change
isOxcCompiler = flags.tscompiler.includes('@oxc-node/core');
isOxcCompiler = flags.tscompiler === '@oxc-node/core/register' || flags.tscompiler.startsWith('@oxc-node/core/');
References
  1. When filtering file paths, avoid broad substring checks like 'includes("node_modules")'. Instead, match against exact path segments to prevent incorrect matches on similarly named directories or files.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — addressed in a674b3f. The detection now matches the exact specifier or a @oxc-node/core/ subpath prefix instead of a loose includes(), so a similarly named compiler won't be misdetected as oxc. Also added an in-process test covering the explicit non-oxc --tscompiler register branch.

if (isOxcCompiler) {
// `@oxc-node/core/register` is exported with an `import`-only condition
// (no `require`), so it cannot be CJS-resolved nor `--require`d. Resolve
// the package root through its main entry, then inject register.mjs as a
// single `--import` — this transpiles `.ts` for both CJS and ESM apps.
const oxcRegister = path.join(path.dirname(cjsResolve('@oxc-node/core')), 'register.mjs');
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
flags.tscompiler = oxcRegister;
this.addNodeOptions(`--import "${pathToFileURL(oxcRegister).href}"`);
} else {
// legacy compilers (ts-node, swc, esbuild) expose a CJS register entry
const tsNodeRegister = cjsResolve(flags.tscompiler);
flags.tscompiler = tsNodeRegister;
// should require tsNodeRegister on current process, let it can require *.ts files
// e.g.: dev command will execute egg loader to find configs and plugins
// await importModule(tsNodeRegister);
// let child process auto require ts-node too
this.addNodeOptions(this.formatImportModule(tsNodeRegister));
}
// tell egg loader to load ts file
// see https://github.com/eggjs/egg-core/blob/master/lib/loader/egg_loader.js#L443
this.env.EGG_TYPESCRIPT = 'true';
// set current process.env.EGG_TYPESCRIPT too
process.env.EGG_TYPESCRIPT = 'true';
// load files from tsconfig on startup
this.env.TS_NODE_FILES = process.env.TS_NODE_FILES ?? 'true';
// keep same logic with egg-core, test cmd load files need it
// keep same logic with egg-core, test cmd load files need it.
// oxc-node does not resolve tsconfig `paths`, so tsconfig-paths/register
// is still required alongside every compiler.
// see https://github.com/eggjs/egg-core/blob/master/lib/loader/egg_loader.js#L49
const tsConfigPathsRegister = cjsResolve('tsconfig-paths/register');
this.addNodeOptions(this.formatImportModule(tsConfigPathsRegister));
}
if (this.isESM) {
if (this.isESM && !isOxcCompiler) {
// use ts-node/esm loader on esm
let esmLoader = cjsResolve('ts-node/esm');
// ES Module loading with absolute path fails on windows
Expand Down
4 changes: 2 additions & 2 deletions tools/egg-bin/src/commands/snapshot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,8 @@ export default class Snapshot<T extends typeof Snapshot> extends BaseCommand<T>

async #spawnNode(nodeArgs: readonly string[], extraEnv: NodeJS.ProcessEnv = {}): Promise<void> {
// Run the self-contained bundle with a clean env: start from process.env, NOT
// this.env. BaseCommand.#afterInit injects NODE_OPTIONS=--loader ts-node/esm
// (plus ts-node/register, tsconfig-paths) into this.env for TypeScript apps;
// this.env. BaseCommand.#afterInit injects NODE_OPTIONS=--import @oxc-node/core/register
// (plus tsconfig-paths) into this.env for TypeScript apps;
// applying that to `node --build-snapshot worker.js` would pull a non-bundled
// loader into the snapshot build. process.env never carries that injection.
const env = { ...process.env, ...extraEnv };
Expand Down
8 changes: 4 additions & 4 deletions tools/egg-bin/src/commands/test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -161,10 +161,10 @@ export default class Test<T extends typeof Test> extends BaseCommand<T> {
}

// Propagate NODE_OPTIONS from this.env to process.env so vitest fork
// workers inherit them (e.g. ts-node/esm loader for TypeScript support).
// Also disable Node.js native type stripping when TypeScript loader is active,
// because native type stripping can't handle decorators and runs before
// custom ESM loaders like ts-node/esm.
// workers inherit them (e.g. the @oxc-node/core/register loader for
// TypeScript support). Also disable Node.js native type stripping when a
// TypeScript loader is active, because native type stripping can't handle
// decorators and runs before custom module hooks like @oxc-node/core.
if (this.env.NODE_OPTIONS) {
let nodeOptions = this.env.NODE_OPTIONS;
if (flags.typescript && !nodeOptions.includes('--no-experimental-strip-types')) {
Expand Down
9 changes: 4 additions & 5 deletions tools/egg-bin/test/coffee.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,11 @@ import coffee from 'coffee';
const coffeeFork = {
fork(modulePath: string, args: string[], options: ForkOptions = {}): ReturnType<typeof coffee.fork> {
options.execArgv = [
// '--require', 'ts-node/register/transpile-only',
// oxc-node registers a CJS require hook + an ESM module.register() hook
// from one `--import`, so the forked egg-bin CLI (TypeScript) boots much
// faster than the old `ts-node/register` + `ts-node/esm` loader pair.
'--import',
'ts-node/register/transpile-only',
'--no-warnings',
'--loader',
'ts-node/esm',
'@oxc-node/core/register',
...(options.execArgv ?? []),
];
options.env = {
Expand Down
11 changes: 6 additions & 5 deletions tools/egg-bin/test/my-egg-bin.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,11 @@ describe('test/my-egg-bin.test.ts', () => {
.end();
});

// Each forked `egg-bin` spawn pays a slow ts-node/esm startup (~50s on
// Windows CI). Keep these as one fork per case so no single test sums multiple
// sequential spawns and blows the timeout — splitting was the fix for the
// flaky `Test bin (windows-latest)` job.
// Each forked `egg-bin` spawn pays a TypeScript loader startup cost (smaller
// now that the CLI boots via @oxc-node/core/register instead of ts-node/esm,
// but still non-trivial on Windows CI). Keep these as one fork per case so no
// single test sums multiple sequential spawns and blows the timeout —
// splitting was the fix for the flaky `Test bin (windows-latest)` job.
it('should my-egg-bin nsp -h success', async () => {
await coffee
.fork(eggBin, ['nsp', '-h'], { cwd })
Expand Down Expand Up @@ -69,7 +70,7 @@ describe('test/my-egg-bin.test.ts', () => {
// .debug()
.expect('stdout', /Run the development server with my-egg-bin/)
.expect('stdout', /listening port, default to 7001/)
.expect('stdout', /TypeScript compiler, like ts-node\/register/)
.expect('stdout', /TypeScript compiler, like @oxc-node\/core\/register/)
.expect('code', 0)
.end();
});
Expand Down
Loading