feat(bundler): scaffold @eggjs/egg-bundler package#5878
feat(bundler): scaffold @eggjs/egg-bundler package#5878killagu wants to merge 1 commit intosplit/06-core-set-bundle-storefrom
Conversation
Introduce the @eggjs/egg-bundler workspace package skeleton under tools/egg-bundler. Subsequent commits add ExternalsResolver, ManifestLoader, the generate-manifest subprocess, and the public BundlerConfig API on top of this scaffold. Wiring: - Added tools/egg-bundler to root tsconfig.json references. - Added @utoo/pack to the workspace catalog (tsx already present). - Root tsdown.config.ts: externalize @utoo/pack and any .node binary so rolldown does not attempt to analyse the NAPI-RS prebuilt binaries shipped by @utoo/pack and its transitive domparser-rs. 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 introduces a new tool, @eggjs/egg-bundler, designed to bundle Egg.js applications using @utoo/pack. The changes include the new package's configuration files, source entry, and updates to the workspace catalog and root TypeScript/bundler configurations to support the new tool and handle NAPI-RS binaries. Feedback focuses on correcting the typecheck script, resolving the conflict between the private flag and publishing configurations, improving the exports field for better type resolution, and correcting a likely typo in the required Node.js version.
| }, | ||
| "scripts": { | ||
| "test": "vitest run", | ||
| "typecheck": "tsgo --noEmit", |
There was a problem hiding this comment.
The typecheck script uses tsgo, but this package is not listed in devDependencies or the workspace catalog. Is this a typo for tsc? If tsgo is a custom tool, it should be added to the dependencies to ensure consistent behavior in CI.
| "typecheck": "tsgo --noEmit", | |
| "typecheck": "tsc --noEmit", |
| { | ||
| "name": "@eggjs/egg-bundler", | ||
| "version": "0.0.0", | ||
| "private": true, |
There was a problem hiding this comment.
The package is marked as private: true, but it contains a publishConfig and a prepublishOnly script. If this package is intended to be published to a registry, private should be set to false. If it is meant to be an internal workspace tool only, the publishConfig and prepublishOnly script are unnecessary.
| "exports": { | ||
| ".": "./src/index.ts", | ||
| "./package.json": "./package.json" | ||
| }, |
There was a problem hiding this comment.
The exports field is missing the types condition. While pointing directly to .ts files works within the workspace, it's better practice to be explicit so TypeScript can resolve types correctly in all environments.
"exports": {
".": {
"types": "./src/index.ts",
"import": "./src/index.ts"
},
"./package.json": "./package.json"
},| "exports": { | ||
| ".": "./dist/index.js", | ||
| "./package.json": "./package.json" | ||
| } |
There was a problem hiding this comment.
The publishConfig.exports field is missing the types condition. Without it, consumers of the published package might have trouble resolving the type definitions even if the top-level types field is present.
"exports": {
".": {
"types": "./dist/index.d.ts",
"import": "./dist/index.js"
},
"./package.json": "./package.json"
}| "egg": "workspace:*" | ||
| }, | ||
| "engines": { | ||
| "node": ">=22.18.0" |
There was a problem hiding this comment.
The Node.js engine version node: >=22.18.0 seems incorrect as Node 22.18.0 has not been released yet (the current latest is 22.13.x). Did you mean 20.18.0 or perhaps 22.13.0?
| "node": ">=22.18.0" | |
| "node": ">=22.13.0" |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Scaffolds a new tools/egg-bundler/ workspace package and wires it into the repo build/tooling config, including new bundling and test config, plus adding @utoo/pack to the workspace catalog.
Changes:
- Add new
@eggjs/egg-bundlerpackage skeleton (package.json, tsconfig, tsdown, vitest config, gitignore). - Register the new package in root TS project references.
- Update bundler config/catalog to support
@utoo/pack(incl. externalizing.nodebinaries).
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tsdown.config.ts | Externalizes @utoo/pack and .node binaries to avoid bundler issues with native addons. |
| tsconfig.json | Adds tools/egg-bundler to TS project references. |
| tools/egg-bundler/vitest.config.ts | Adds Vitest configuration for the new package. |
| tools/egg-bundler/tsdown.config.ts | Adds tsdown build config for the new package. |
| tools/egg-bundler/tsconfig.json | Extends root tsconfig for the new package. |
| tools/egg-bundler/src/index.ts | Adds placeholder entrypoint. |
| tools/egg-bundler/package.json | Defines the new workspace package metadata, scripts, and dependencies. |
| tools/egg-bundler/.gitignore | Ignores build outputs and local bundle artifacts. |
| pnpm-workspace.yaml | Adds @utoo/pack to the workspace catalog. |
| ".": "./src/index.ts", | ||
| "./package.json": "./package.json" | ||
| }, | ||
| "publishConfig": { | ||
| "access": "public", | ||
| "exports": { | ||
| ".": "./dist/index.js", |
There was a problem hiding this comment.
The runtime export map currently points "." to ./src/index.ts while the package is "type": "module". In Node, exports takes precedence over main/module, so consumers will resolve to the TypeScript source (which Node cannot execute without a TS loader), even though main points at dist. Consider making exports resolve to dist (and include a types condition if needed), and reserve any “source export” behavior for tooling-only workflows (e.g., separate dev conditions) to avoid runtime resolution failures.
| ".": "./src/index.ts", | |
| "./package.json": "./package.json" | |
| }, | |
| "publishConfig": { | |
| "access": "public", | |
| "exports": { | |
| ".": "./dist/index.js", | |
| ".": { | |
| "types": "./dist/index.d.ts", | |
| "default": "./dist/index.js" | |
| }, | |
| "./package.json": "./package.json" | |
| }, | |
| "publishConfig": { | |
| "access": "public", | |
| "exports": { | |
| ".": { | |
| "types": "./dist/index.d.ts", | |
| "default": "./dist/index.js" | |
| }, |
| import { defineProject, type UserWorkspaceConfig } from 'vitest/config'; | ||
|
|
||
| const config: UserWorkspaceConfig = defineProject({ | ||
| test: { | ||
| testTimeout: 20000, | ||
| include: ['test/**/*.test.ts'], | ||
| exclude: ['**/node_modules/**', '**/dist/**'], | ||
| }, | ||
| }); |
There was a problem hiding this comment.
vitest.config.ts is typically a project config (via defineConfig/UserConfig), while defineProject/UserWorkspaceConfig are intended for workspace project entries (usually referenced from a vitest.workspace.ts). Using workspace typing here can be confusing and may not match how Vitest expects to load a per-package config. If this file is meant to be the package’s direct config for vitest run, consider switching to defineConfig and the project config type to match Vitest conventions.
New
tools/egg-bundler/package with package.json, tsconfig, tsdown.config (copy rule for generate-manifest.mjs), vitest.config. Adds@utoo/packto workspace catalog, externalizes.nodebinaries in root tsdown.config, and registers in root tsconfig references.Part of #5863 split. Tracking: #5871. Stacked on: A3 (setBundleStore)
🤖 Generated with Claude Code