feat(bundler): add generate-manifest subprocess with tsx injection#5881
feat(bundler): add generate-manifest subprocess with tsx injection#5881killagu wants to merge 1 commit intosplit/09-bundler-manifest-loaderfrom
Conversation
…tion Add scripts/generate-manifest.mjs which boots the egg framework to produce a manifest via app.loader.generateManifest() and writes it through ManifestStore.write(). The script accepts both: - `framework` (absolute package dir) — forwarded to framework.start() so egg's resolveFrameworkClasses() keeps working via importResolve. - `frameworkEntry` (file:// URL) — used for the subprocess's initial dynamic import(), which is the only way to load a workspace-linked framework whose `exports` map points at a TypeScript source. ManifestLoader changes to drive the script: - Switch from fork() to spawn(): tsx's ESM loader has resolver issues inside Node 22's IPC hooks-worker, causing workspace-linked packages with `exports: "./src/*.ts"` to fall back to directory resolution. - #buildExecArgv() injects `--import=file://.../tsx/dist/esm/index.mjs` unless a tsx loader is already present. The detection regex accepts bare specifiers and absolute file:// URLs to prevent recursive duplication. - #resolveFrameworkEntryUrl() reads the framework package.json's exports['.'] (string or conditional) and falls back to module/main, returning an absolute file:// URL. tsdown.config.ts: land the script at dist/scripts/generate-manifest.mjs via a copy rule so the subprocess resolves correctly in both dev (workspace link) and publish (tgz install) forms. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the manifest generation process by switching from fork to spawn to avoid IPC-related ESM resolution issues in Node 22. It introduces a new generate-manifest.mjs script and logic to resolve framework entry points from package.json. Reviewers identified that the exports resolution logic fails for shorthand condition maps and noted a potential risk of exceeding command-line argument length limits when passing JSON payloads.
| } else if (pkg.exports && typeof pkg.exports === 'object') { | ||
| const dot = (pkg.exports as Record<string, unknown>)['.']; | ||
| if (typeof dot === 'string') { | ||
| entryRel = dot; | ||
| } else if (dot && typeof dot === 'object') { | ||
| const cond = dot as Record<string, unknown>; | ||
| for (const key of ['import', 'module', 'default'] as const) { | ||
| const val = cond[key]; | ||
| if (typeof val === 'string') { | ||
| entryRel = val; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The exports resolution logic is incomplete. According to the Node.js specification, if exports is an object and does not contain keys starting with ., the object itself is treated as the mapping for the root (.) entry. The current implementation only looks for a . key, which will fail for packages using the shorthand condition map (e.g., "exports": { "import": "..." }).
} else if (pkg.exports && typeof pkg.exports === 'object') {
const keys = Object.keys(pkg.exports);
const isConditionalMap = keys.length > 0 && !keys.some(k => k.startsWith('.'));
const dot = isConditionalMap ? pkg.exports : (pkg.exports as Record<string, unknown>)['.'];
if (typeof dot === 'string') {
entryRel = dot;
} else if (dot && typeof dot === 'object') {
const cond = dot as Record<string, unknown>;
for (const key of ['import', 'module', 'require', 'default'] as const) {
const val = cond[key];
if (typeof val === 'string') {
entryRel = val;
break;
}
}
}
}| // packages with `exports: "./src/*.ts"` to fall back to directory resolution. | ||
| await new Promise<void>((resolve, reject) => { | ||
| const child = fork(scriptPath, [JSON.stringify(payload)], { | ||
| const child = spawn(process.execPath, [...execArgv, scriptPath, JSON.stringify(payload)], { |
There was a problem hiding this comment.
Passing a large JSON string as a command-line argument can potentially exceed the operating system's maximum argument length limit (ARG_MAX). While the current payload might be small, it's safer to pass this data via an environment variable or stdin to ensure scalability as the project grows.
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a dedicated manifest-generation subprocess script and updates ManifestLoader to use spawn() with tsx ESM loader injection, enabling manifest generation when workspace-linked frameworks export TypeScript sources.
Changes:
- Add
src/scripts/generate-manifest.mjsand ensure it’s copied intodist/. - Switch manifest generation from
fork()tospawn()and injecttsx/esmvia--import=...when needed. - Resolve and pass an explicit
file://framework entry URL derived frompackage.jsonexports['.'].
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| tools/egg-bundler/tsdown.config.ts | Copies the new manifest-generation script into the build output. |
| tools/egg-bundler/src/scripts/generate-manifest.mjs | New subprocess entrypoint that starts the framework and writes the manifest. |
| tools/egg-bundler/src/lib/ManifestLoader.ts | Uses spawn() + --import injection and resolves frameworkEntry from exports. |
| env: this.#env, | ||
| scope: this.#scope, | ||
| }; | ||
| debug('fork generate-manifest: %o', payload); |
There was a problem hiding this comment.
The debug message still says 'fork' but the implementation now uses spawn(). Update the log string to match the actual behavior so debugging output is not misleading.
| debug('fork generate-manifest: %o', payload); | |
| debug('spawn generate-manifest: %o', payload); |
| if (options.frameworkEntry) { | ||
| framework = await import(options.frameworkEntry); | ||
| } else if (options.framework) { | ||
| framework = await import(options.framework); | ||
| } else { | ||
| framework = await import('egg'); | ||
| } |
There was a problem hiding this comment.
options.framework (as constructed by ManifestLoader) is an absolute package directory path, which is not a reliable ESM import() specifier and can fail to resolve. Since frameworkEntry is now the supported import target, consider removing the options.framework import fallback, or convert absolute paths to a file:// URL before importing (and/or fall back by resolving an entry file from that directory).
| #resolveFrameworkEntryUrl(): string { | ||
| const frameworkDir = this.#resolveFrameworkPath(); | ||
| const pkgJsonPath = path.join(frameworkDir, 'package.json'); | ||
| try { | ||
| const pkg = JSON.parse(fs.readFileSync(pkgJsonPath, 'utf-8')) as { | ||
| exports?: Record<string, unknown> | string; | ||
| main?: string; | ||
| module?: string; | ||
| }; | ||
| let entryRel: string | undefined; | ||
| if (typeof pkg.exports === 'string') { | ||
| entryRel = pkg.exports; | ||
| } else if (pkg.exports && typeof pkg.exports === 'object') { | ||
| const dot = (pkg.exports as Record<string, unknown>)['.']; | ||
| if (typeof dot === 'string') { | ||
| entryRel = dot; | ||
| } else if (dot && typeof dot === 'object') { | ||
| const cond = dot as Record<string, unknown>; | ||
| for (const key of ['import', 'module', 'default'] as const) { | ||
| const val = cond[key]; | ||
| if (typeof val === 'string') { | ||
| entryRel = val; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| entryRel = entryRel ?? pkg.module ?? pkg.main; | ||
| if (!entryRel) { | ||
| throw new Error(`[@eggjs/egg-bundler] framework package ${pkgJsonPath} has no resolvable entry`); | ||
| } | ||
| return pathToFileURL(path.resolve(frameworkDir, entryRel)).href; |
There was a problem hiding this comment.
This exports resolution only handles a shallow set of shapes. Real-world exports['.'] often contains nested conditional objects (e.g. import: { development: '...', default: '...' }) or other patterns; in those cases entryRel will fall back to module/main and may point at the wrong file (or be missing), defeating the purpose of frameworkEntry. Consider implementing a small recursive resolver that walks the exports['.'] target and returns the first string for preferred conditions (e.g. import, then default), and validate that the resolved target is a relative path (./...) before path.resolve().
| #buildExecArgv(): string[] { | ||
| const base = this.#execArgv ?? process.execArgv; | ||
| // Detect any prior tsx loader injection so recursive forks don't append duplicates. | ||
| // Accepts either bare specifier (`tsx`, `tsx/esm`) or absolute file:// URL pointing | ||
| // inside a tsx package (e.g. `.../tsx/dist/esm/index.mjs`). | ||
| const hasTsxLoader = base.some((arg) => /(^|[=\s])tsx($|\/|\s)|\/tsx(@[^/]*)?\/(dist\/)?esm\//.test(arg)); | ||
| if (hasTsxLoader) return [...base]; |
There was a problem hiding this comment.
The tsx-injection detection regex is not portable to Windows paths (backslashes) and can miss/duplicate injections depending on how Node flags are represented (--import, --import=..., file URLs vs paths). A more robust approach is to explicitly parse base for --import / --import=... values and check whether the resolved specifier/path is tsx, tsx/esm, or ends with tsx/dist/esm/index.mjs, normalizing separators (or comparing as file:// URLs).
| #buildExecArgv(): string[] { | |
| const base = this.#execArgv ?? process.execArgv; | |
| // Detect any prior tsx loader injection so recursive forks don't append duplicates. | |
| // Accepts either bare specifier (`tsx`, `tsx/esm`) or absolute file:// URL pointing | |
| // inside a tsx package (e.g. `.../tsx/dist/esm/index.mjs`). | |
| const hasTsxLoader = base.some((arg) => /(^|[=\s])tsx($|\/|\s)|\/tsx(@[^/]*)?\/(dist\/)?esm\//.test(arg)); | |
| if (hasTsxLoader) return [...base]; | |
| #isTsxImportTarget(specifier: string): boolean { | |
| if (specifier === 'tsx' || specifier === 'tsx/esm') return true; | |
| let normalized = specifier; | |
| if (specifier.startsWith('file://')) { | |
| try { | |
| normalized = fileURLToPath(specifier); | |
| } catch { | |
| return false; | |
| } | |
| } | |
| normalized = normalized.replaceAll('\\', '/'); | |
| return normalized.endsWith('/tsx/dist/esm/index.mjs'); | |
| } | |
| #hasTsxLoader(base: string[]): boolean { | |
| for (let i = 0; i < base.length; i++) { | |
| const arg = base[i]; | |
| let importTarget: string | undefined; | |
| if (arg === '--import') { | |
| importTarget = base[i + 1]; | |
| i++; | |
| } else if (arg.startsWith('--import=')) { | |
| importTarget = arg.slice('--import='.length); | |
| } | |
| if (importTarget && this.#isTsxImportTarget(importTarget)) { | |
| return true; | |
| } | |
| } | |
| return false; | |
| } | |
| #buildExecArgv(): string[] { | |
| const base = this.#execArgv ?? process.execArgv; | |
| // Detect any prior tsx loader injection so recursive forks don't append duplicates. | |
| // Accepts either bare specifier (`tsx`, `tsx/esm`) or absolute file:// URL pointing | |
| // inside a tsx package (e.g. `.../tsx/dist/esm/index.mjs`). | |
| if (this.#hasTsxLoader(base)) return [...base]; |
| console.log('[bundler-manifest] extensions: %d', extensionCount); | ||
|
|
||
| await app.close(); | ||
| process.exit(0); |
There was a problem hiding this comment.
Calling process.exit(0) after awaited async work can still terminate the process abruptly (e.g., before streams flush in some environments or before any remaining microtasks complete). Prefer allowing natural exit (returning from main) or setting process.exitCode = 0 to make shutdown behavior more predictable.
| process.exit(0); | |
| return; |
Adds
scripts/generate-manifest.mjs(59 lines). ManifestLoader usesspawn()(not fork) with--importto inject tsx loader, resolves framework entry frompackage.json exports['.'], passes explicitfile://URL asframeworkEntry.Part of #5863 split. Tracking: #5871.
🤖 Generated with Claude Code