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: |
1b13d61
|
| Status: | 🚫 Build failed. |
Deploying egg with
|
| Latest commit: |
1b13d61
|
| 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.39% -0.50%
==========================================
Files 666 651 -15
Lines 18841 18693 -148
Branches 3636 3627 -9
==========================================
- Hits 16182 15962 -220
- Misses 2296 2355 +59
- Partials 363 376 +13 ☔ 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 <[email protected]>
npm does not understand catalog: protocol; ut install handles it correctly. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
…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 <[email protected]>
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 <[email protected]>
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 <[email protected]>
…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 <[email protected]>
The utoo CI environment does not have pnpm in PATH, causing publint's auto-detected `pnpm pack` command to fail during build. Co-Authored-By: Claude Opus 4.6 <[email protected]>
5 tasks
Under ut install the .oxfmtrc.json walk-up discovery seems to miss the printWidth: 120 setting, causing root package.json line 34 (115 chars) to exceed the implicit default and fail formatting. Pass the config path explicitly. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
ut install --from pnpm rewrites package.json by appending a workspaces field at the bottom (after packageManager). This trips oxfmt because oxfmt expects workspaces to come immediately after repository per npm field-order convention. Add a 'git checkout -- package.json' step right after every 'ut install' to discard ut's rewrite and keep oxfmt happy. Also revert the explicit -c .oxfmtrc.json on fmtcheck (no longer needed, since the real cause was ut's rewrite, not config discovery). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The real issue was ut install rewriting package.json (fixed in previous commit), not config discovery. Restore the simpler fmtcheck command. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…install ut install --from pnpm intentionally rewrites package.json to add the npm-style workspaces field (that's its whole purpose). Reverting it defeats the migration tool. Instead, ignore the root package.json in .oxfmtrc.json so oxfmt does not enforce field ordering on it. Other package.json files in workspaces are still checked. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
elrrrrrrr
changed the title
ut now handles workspace package linking correctly, so the manual symlink-creation step (added when ut copied workspace packages instead of symlinking them) is no longer needed. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Try running CI without the require.resolve fallback. If something breaks (likely tsconfig-paths/register or similar CJS subpath imports), this commit will be reverted. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…eeded" This reverts commit 86e33e5.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…eeded" This reverts commit e1f57cf.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This PR focuses on migrating the main CI workflow to ut. The E2E workflow needs catalog: and workspace: protocol resolution which ut does not provide yet (ut pm-pack ships unresolved protocols), and pnpm -r pack handles this natively. Keep pnpm for E2E and revisit once ut adds publishConfig + protocol resolution. Also remove ecosystem-ci/pack-all.mjs which was added solely as a workaround for the pnpm-less E2E. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Node.js 24's ESM resolver returns extensionless paths for CJS packages
without an `exports` field, e.g. import.meta.resolve('tsconfig-paths/register')
returns '.../register' instead of '.../register.js'. fs.statSync then
fails and importResolve throws.
Fall back to tryToResolveFromFile to probe common extensions
(.mjs/.js/.cjs/.ts) when stat says the resolved path is not a file.
This is a much smaller fix than the previous 70-line rewrite — it only
adds the extension-probe in the failing branch, leaving the rest of
importResolve unchanged.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
ut install --from pnpm reads pnpm-workspace.yaml which already declares the vite → rolldown-vite override. The duplicate npm-style overrides in package.json is unnecessary. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ehavior The previous fix only handled Node 25's behavior of returning an extensionless path. Node 24 instead throws ERR_MODULE_NOT_FOUND from import.meta.resolve for CJS subpaths without an exports field (e.g. tsconfig-paths/register). Add a require.resolve fallback in the catch branch as well, so both Node 24 (throw) and Node 25 (extensionless return) cases are handled. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…solve The previous ESM branch called import.meta.resolve(filepath) without a parentURL, which has two problems: 1. It silently ignores the caller-supplied paths option, always resolving from this module's own context. 2. Node 24 throws ERR_MODULE_NOT_FOUND for CJS subpaths without an exports field (e.g. tsconfig-paths/register), and Node 25 returns the extensionless path which then fails fs.statSync. require.resolve handles both cases correctly: - honors caller-supplied paths - walks up the node_modules chain via Node.js standard module resolution algorithm - reads the exports field on modern Node.js - auto-appends extensions for legacy CJS subpaths - resolves .ts/.mjs files alike Verified locally with all egg-bin importResolve targets. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
I removed nodeMajorVersion when removing supportImportMetaResolve, but it's also used at line 66 for the ts enum support detection. Restore the const. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
This reverts commit 8c32970.
…ssage require.resolve throws 'Cannot find module' (CJS resolver format) while import.meta.resolve throws 'Cannot find package' (ESM resolver format). After switching importResolve to use require.resolve, update the test regex to accept either wording. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…r both Node 24/25 The previous refactor switched to require.resolve completely, which broke cnpmcore E2E because import.meta.resolve goes through the ts-node ESM loader hook (which maps .js → .ts), while require.resolve goes through the CJS resolver chain (where ts-node/register doesn't do the .js → .ts substitution). Restore the import.meta.resolve as the primary path so the ts-node loader hook still applies, and add require.resolve fallback only when import.meta.resolve fails (Node 24 throws for CJS subpaths without exports) or returns an extensionless path (Node 25 case for the same type of CJS subpath). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The require.resolve fallback (Node 24 throw case) and extensionless probing fallback (Node 25 case) were added as defensive code but are not exercised by CI. cnpmcore E2E (Node 24 + ts-node ESM loader) verified that import.meta.resolve alone is sufficient — failure count remained 1 (unrelated EdgedriverBinary network flaky test) with or without the fallbacks. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
The previous refactor (f170210) dropped this fallback assuming it was unused, but egg-bin tests on Node 24 (both ubuntu and windows) regressed because import.meta.resolve throws ERR_MODULE_NOT_FOUND for `tsconfig-paths/register` (a CJS package without `exports`). Restore the require.resolve fallback inside the catch block so caller-supplied `paths` are honored and legacy CJS subpaths resolve via Node's standard CJS resolver. The Node 25 extensionless probing fallback (Fallback 2) remains removed since no test exercised it. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Expand the inline comment for the require.resolve fallback inside
import.meta.resolve's catch block. Mark it explicitly as "Fallback 1"
and explain the two combined reasons it is reached:
1. Node 22+ ESM resolver requires explicit file extensions for
subpaths of packages without an `exports` field.
2. The manual node_modules walk only checks immediate children
plus two pnpm sibling levels, not the full directory tree, so
workspace-hoisted dependencies fall through to import.meta.resolve.
Also note the public-API constraint: removing this fallback would be
a breaking change for any `@eggjs/utils` consumer (internal or
downstream) that resolves a bare CJS subpath like
`tsconfig-paths/register`.
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
…ary fix Picks up cnpm/cnpmcore#1026, which replaces the dead Azure Blob XML listing API with directly-generated per-platform download URLs on msedgedriver.microsoft.com. This unblocks the chore-ut-ci E2E run, which has been failing on `EdgedriverBinary.test.ts > should work` with `items.length >= 3` since 2026-04-08 04:19 UTC. Old hash: e82df3f4 (pre-fix) New hash: 98463c33 (cnpmcore release v4.32.1, contains the merged PR) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
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) <[email protected]>
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) ✅