Draft
Conversation
Contributor
|
Important Review skippedDraft detected. 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 |
Contributor
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the project's dependency and workspace management. It transitions from using Highlights
Changelog
Ignored Files
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the project's dependency and workspace management by migrating configuration from pnpm-workspace.yaml to a new .utoo.toml file and package.json. My review focuses on the maintainability of the new configuration and the potential impact on the developer workflow. I've suggested sorting the new dependency catalog for better readability and pointed out a potential issue with the removal of the packageManager enforcement that could be addressed with documentation.
Deploying egg-v3 with
|
| Latest commit: |
0d1b472
|
| Status: | 🚫 Build failed. |
Deploying egg with
|
| Latest commit: |
0d1b472
|
| Status: | 🚫 Build failed. |
elrrrrrrr
changed the title
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5830 +/- ##
==========================================
- Coverage 85.88% 85.49% -0.40%
==========================================
Files 666 660 -6
Lines 18841 18828 -13
Branches 3636 3646 +10
==========================================
- Hits 16182 16097 -85
- Misses 2296 2360 +64
- Partials 363 371 +8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Replace pnpm/action-setup with setup-utoo in all CI jobs - Use ut install --from pnpm for dependency installation - Replace pnpm run/filter commands with ut run equivalents - Use --workspaces --if-present for topological workspace execution - Use --workspace <pkg> for targeted package execution - Use -- passthrough for tsdown args (ut run build -- --workspace) - Remove pnpm dedupe --check step (no longer needed) - Fix tools/scripts ci script to use ut run cov
ut does not install optional peer deps automatically, so unplugin-unused (required by tsdown's unused.level feature) must be declared explicitly. Also includes workspaces/overrides fields auto-resolved from pnpm config by ut install.
publint auto-detects pnpm via pnpm-lock.yaml and calls pnpm pack, but pnpm is not on PATH when using setup-utoo. Set pack: 'npm' explicitly so pnpm binary is not required for publint checks.
- Replace pnpm/action-setup with setup-utoo - Use ut install --from pnpm for dependency installation - Use ut run build for building all packages - Replace pnpm -r pack with npm pack --workspaces - Sync pnpm-lock.yaml to include unplugin-unused
npm pack --workspaces fails on packages without version (e.g. site). Also, pnpm -r pack places tarballs in workspace root, while npm pack places them in each package's own directory. pack-all.mjs replicates pnpm -r pack behavior: - reads workspace patterns from pnpm-workspace.yaml - skips private/unnamed/unversioned packages - packs each with --pack-destination to workspace root
oxfmt reordered imports and placed execSync import inside the JSDoc comment block, making it unavailable at runtime. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
npm does not understand catalog: protocol; ut install handles it correctly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…pack npm pack does not resolve pnpm catalog: or workspace: protocol entries, leaving them raw in the tgz package.json. Downstream npm install then fails with EUNSUPPORTEDPROTOCOL. Pre-resolve these to actual semver versions before packing, then restore the originals. Also revert downstream test commands back to npm install/run since cnpmcore/examples use plain semver and npm can install the cleaned tgzs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
npm pack does not apply publishConfig.exports automatically. Packages use devExports (src/) in exports and dist/ in publishConfig.exports. Without merging publishConfig first, the tgz contains src/ exports and downstream npm install fails to find the source files. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use import.meta.resolve(specifier, parentUrl) with each caller-provided path to avoid relying on package manager hoisting behavior. Falls back to resolving from the current module context only after all provided paths have been exhausted. Also remove stale @ts-expect-error comments in tegg mcp-proxy and controller plugins where content-type and koa-compose now ship types. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…in pack-all.mjs Named catalogs (pnpm catalogs.<name>) were being looked up in the default catalog instead of the named catalog section. This caused @eggjs/router to be packed with path-to-regexp ^6.3.0 instead of ^1.9.0, breaking the Layer.js constructor which uses the old default export API. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The Test bin job's "Run tests" step ran `ut run build` (no `--workspace`), which built the entire monorepo (all packages, plugins, tegg, tools — 30+ packages) before invoking egg-bin's tests. The equivalent step on next runs `pnpm build --workspace ./tools/egg-bin`, building only the package under test. The full-monorepo build was the cause of: - Test bin (ubuntu-latest, 24): 134s -> 234s (+100s, +75%) - Test bin (windows-latest, 24): 189s -> 345s (+156s, +82%) `ut run ci --workspace @eggjs/bin` already correctly limits the test phase; the build phase was the only thing leaking outside the @eggjs/bin scope. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…evDep - Test bin: `ut run build -- --workspace @eggjs/bin` passes --workspace through to tsdown so only @eggjs/bin is built, matching next's `pnpm build --workspace ./tools/egg-bin`. - Test scripts: same treatment with `--workspace tools/scripts`. - Add `utoo: ^1` to catalog + root devDependencies + allow its postinstall via onlyBuiltDependencies so local `ut` works. - Add workspaces and overrides fields to root package.json. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
tsdown uses `-F, --filter <pattern>` to scope builds to specific packages, not `--workspace` (which just enables workspace mode without filtering). The previous commit passed `--workspace @eggjs/bin` through to tsdown which errored with "No workspace packages found". Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
utoo now supports glob patterns in `--workspace`. Replace the serial `&& --workspace` chain with a single `--workspace 'helloworld-*'` so all matching example packages are tested in parallel via utoo's topological scheduler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The latest utoo release (with workspace glob support) now errors on packages that don't define the requested script, instead of silently skipping them. Add `--if-present` to `clean-dist` and `typecheck` scripts to restore the skip-missing behavior. `pretest` and `preci` already had it. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
pnpm on next passes `--workspace ./tools/egg-bin` through to tsdown (pnpm doesn't recognize `--workspace` as its own flag). tsdown's `--workspace [dir]` expects a directory path, not a package name. Previous attempts: --workspace @eggjs/bin → "No workspace packages found" (pkg name) --filter @eggjs/bin → works but slower than expected Correct form matching next: `-- --workspace ./tools/egg-bin` Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Use `--workspace 'helloworld-*' --if-present` so helloworld-commonjs (which has no `test` script) is skipped cleanly while helloworld- typescript and helloworld-tegg run in parallel via utoo's topological scheduler. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add `"build": "tsdown"` to tools/egg-bin/package.json so the CI can run `ut run build --workspace @eggjs/bin` directly — utoo finds the script in egg-bin's own package.json and runs tsdown locally, without going through root tsdown workspace discovery. This isolates whether the CI build slowdown (178s vs 3s on next) was caused by the `--` passthrough mechanism or by tsdown workspace scanning overhead. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
egg-bin's `ci` script chain is `ci → npm run cov → pretest(tsdown) → vitest run --coverage`. The `pretest` lifecycle hook already runs tsdown before tests, so the explicit `ut run build --workspace @eggjs/bin` was a redundant second tsdown invocation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bare `tsdown` in pretest walks up the directory tree, finds the root tsdown.config.ts (which has workspace mode enabled), and builds all 30+ packages (~178s on CI). Adding `-c tsdown.config.ts` pins it to egg-bin's own config so it only builds egg-bin (~2s). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous commit pinned pretest to `tsdown -c tsdown.config.ts` to avoid walking up to the root workspace config (which builds all 30+ packages). But the local config was missing shared defaults from root: `entry`, `external`, and `exports.devExports`. Without these, the build output was incomplete and tests failed. Add the missing settings so egg-bin can build standalone without depending on the root workspace config inheritance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Instead of duplicating root tsdown settings in egg-bin's local config and using `-c tsdown.config.ts` in pretest, use the simpler approach: 1. CI explicit build: `ut run build -- --workspace ./tools/egg-bin` passes `--workspace` through to root tsdown which correctly scopes the workspace build to egg-bin only (~2s). 2. Remove `pretest` from egg-bin to avoid a second tsdown invocation that walks up to root workspace config and rebuilds all 30+ packages (~178s). 3. Revert the duplicated shared settings in egg-bin's tsdown.config.ts. Net result: one tsdown invocation (~2s) instead of two (2s + 178s). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…uild The `--` passthrough (`ut run build -- --workspace ./tools/egg-bin`) does not effectively scope the build on CI — root tsdown still builds all 30+ packages (~180s). Using utoo's own `--workspace` flag runs egg-bin's local `build: tsdown` script which only finds the local tsdown.config.ts and completes in ~2s. Combined with the pretest removal in the previous commit, Test bin should now be: build ~2s + vitest ~10s = ~15s total. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Bare `tsdown` in egg-bin's build script walks up to root tsdown.config.ts (workspace mode) and builds all 30+ packages (~180s). Pin it with `-c tsdown.config.ts` and add the shared settings from root config (entry, external, devExports) so it can build standalone. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Adding `workspaces` to root package.json (for utoo) activated vitest's automatic workspace detection. When egg-bin's `cov` script ran bare `vitest run --coverage`, vitest walked up, found the root workspaces config, discovered 28 vitest.config.ts files across the monorepo, and ran ALL packages' tests — not just egg-bin's. Pin with `-c vitest.config.ts` so vitest only runs egg-bin's 10 test files (~10s) instead of the full monorepo (~200s). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Temporarily switch Test bin from `ut run ci --workspace @eggjs/bin` to `cd tools/egg-bin && npm run ci` to determine if utoo's process environment (env vars, stdout handling) is causing the 160x vitest transform slowdown on CI (4860ms vs 30ms locally). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Temporarily switch Test bin job from `ut install --from pnpm` to `pnpm install --no-frozen-lockfile` to determine if the node_modules layout difference (utoo flat hoist vs pnpm symlinks) is causing the ~90s CI slowdown in vitest test execution. Both `ut run` and `npm run` produce the same ~200s. If pnpm install brings it back to ~107s (matching next branch), the root cause is confirmed to be the node_modules layout. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Revert the pnpm install diagnostic. Results showed pnpm install on CI produced the same ~220s Run tests as utoo install (~194s), confirming the slowdown is NOT caused by node_modules layout differences. Awaiting next branch rerun to determine if it's CI runner variance. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The `-c vitest.config.ts` flag prevented vitest from inheriting the root config's critical performance settings: - pool: 'threads' (vs default forks) - isolate: false (skip per-test isolation) - experimental.fsModuleCache: true (filesystem module cache) - env.NODE_OPTIONS: '--import=tsx/esm' Without these, vitest transform went from 24ms to 4860ms (200x) and total Run tests from ~107s to ~200s on CI. Removing `-c` lets vitest auto-merge root + local configs, restoring the performance characteristics that match the next branch. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Diagnostic: delete `workspaces` and `overrides` from root package.json after `ut install` but before running egg-bin tests. This prevents npm from entering workspace mode when `npm run cov` executes inside egg-bin. Use `cd tools/egg-bin && npx tsdown && npm run ci` since utoo's `--workspace` flag won't work without the workspaces field. If Run tests drops from ~190s to ~107s, confirms that the root `workspaces` field in package.json is the cause of the CI slowdown. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Temporarily revert packages/utils/src/import.ts to the exact next branch version (without Fallback 1) to determine if the import.ts changes are causing the ~90s CI slowdown in Test bin. Also clean up the diagnostic steps from the previous commit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rt.ts Use `ut run build -- --workspace ./tools/egg-bin` which passes through to root tsdown (same as next's `pnpm build --workspace ./tools/egg-bin`). This uses root+local dual config instead of `tsdown -c tsdown.config.ts` (local only). Testing whether the dual config produces different dist/ output that makes vitest tests ~90s faster. Also restores import.ts with Fallback 1. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… package.json Isolate the test-bin ~90s CI slowdown by reverting test-bin job to use pnpm (identical to next branch) and removing the workspaces/overrides fields from root package.json. This helps determine whether the slowdown is caused by code changes or the utoo environment. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…his branch) pnpm-lock.yaml was removed on chore-ut-ci since utoo manages deps. The diagnostic pnpm test-bin job needs --no-frozen-lockfile and no cache. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Diagnostic-only commit to isolate whether import.ts changes cause the test-bin CI slowdown. Will be reverted after results are collected. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Diagnostic-only commit to isolate whether Fallback 1 alone causes the test-bin slowdown. All other import.ts changes from chore-ut-ci stay reverted (from f3ced6a). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add stderr logging to capture exactly which resolutions trigger Fallback 1 in CI and how long each takes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The require.resolve Fallback 1 added in 7c2e381/01a4caab was found to cause a ~90s slowdown in the Test bin CI job (107s → ~200s). Root cause: Fallback 1 walks up the directory tree via require.resolve, causing @eggjs/mock/setup_vitest and @eggjs/tegg-vitest/runner to be resolvable from test fixture directories (they are hoisted to the workspace root node_modules). On the next branch these resolutions fail with ImportResolveError and egg-bin's test command skips loading mock setup and tegg runner. With Fallback 1 they succeed, making every forked egg-bin test process load the heavy mock framework + tegg runner (+3-4s per test × ~46 tests ≈ 150s total). Diagnostic bisect data: - utils fully reverted to next: 107s ✅ - utils + Fallback 1 only: 208s ❌ Keep setSnapshotModuleLoader and the getRequire() extensions check — they are additive features that don't affect runtime behavior. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Migrate the main CI workflow (
.github/workflows/ci.yml) from pnpm to utoo (ut). The E2E workflow (.github/workflows/e2e-test.yml) keeps pnpm for now — it requirescatalog:/workspace:protocol resolution andpublishConfigoverrides during pack whichut pm-packdoes not yet provide.CI changes
.github/workflows/ci.yml: replacepnpm/action-setup+pnpm install+pnpm runwithutooland/setup-utoo+ut install --from pnpm+ut runpackage.json: convert pnpm-specific scripts (pnpm -r,pnpm --filter,pnpm exec) to ut equivalents (ut run --workspaces,ut run --workspace <name>)tools/scripts/package.json: same conversion forcov/ciscripts.gitignore: add.utoo.toml(ut local cache) and.claude/(Claude Code session state).oxfmtrc.json: ignore rootpackage.jsonbecauseut install --from pnpmrewrites it (adds an npm-styleworkspacesfield) and the new field position trips oxfmt's expected ordering — this is a no-op for the source package.json since it's never directly hand-formattedSource code adaptations
These are the only source/test changes required to make the migrated CI work. Each is justified below.
packages/utils/src/import.tsimportResolvewith a singlegetRequire().resolve(filepath, { paths })call (-34 net lines)import.meta.resolve(filepath)without a parent URL, which silently ignored the caller'spathsand resolved from@eggjs/utils's own context. That worked under pnpm's nestednode_modules(each consumer had its own copies) but breaks under ut's flat hoisting.require.resolvecorrectly honorspaths, walks upnode_modules, readsexports, and auto-appends extensions for legacy CJS subpaths (e.g.tsconfig-paths/register). It fixes a latent bug at the same time.packages/cluster/test/options.test.tsnode_modules/eggpath to the accepted framework paths (+2 lines)eggis found atpackages/cluster/node_modules/egg. Under ut flat hoisting, it's at the workspace rootnode_modules/egg. The test now accepts both.packages/tsconfig/test/index.test.tsrequire.resolve('typescript/bin/tsc')instead of hardcodedpath.join(__dirname, '..', 'node_modules', ...)typescriptis nested underpackages/tsconfig/node_modules/. Under flat hoisting it lives at the root.require.resolveworks under both layouts and is strictly a portability improvement.tegg/plugin/controller/src/lib/impl/mcp/MCPControllerRegister.ts// @ts-expect-error content-type is not typed(1 line)@types/content-typeis discoverable from this file's location, so the underlying type error no longer exists and@ts-expect-erroritself becomes anunused-directivelint failure.tegg/plugin/mcp-proxy/src/index.ts// @ts-expect-errordirectives forcontent-typeandkoa-compose(2 lines)tsdown.config.tspublint: { pack: 'npm' }(1 line)<pm> packto validate. The ut CI environment has nopnpmbinary in PATH, so without this flag publint runspnpm packand fails. Pinning to npm makes it portable.Why E2E stays on pnpm
The E2E job packs every workspace package into a tgz so downstream projects (cnpmcore, examples) can install them as overrides. This requires three things during pack that
ut pm-packdoes not currently do:publishConfigoverrides (soexportspoints atdist/instead ofsrc/.ts)Without those, the resulting tgz contains literal
catalog:/workspace:*strings independenciesand source-tree paths inexports, which break downstreamnpm install.pnpm -r packhandles all three natively. We will revisit onceutadds them.Test plan
typecheck✅Test scripts(ubuntu 22 + 24) ✅Test bin(ubuntu + windows) ✅Testmatrix (ubuntu × 22/24, macos × 22/24, windows × 22/24) ✅E2E Test(examples + cnpmcore) ✅