feat(typings): add shared bundle module loader types#5897
feat(typings): add shared bundle module loader types#5897
Conversation
Add `setBundleModuleLoader()` which intercepts `importModule()` BEFORE `importResolve()` so bundled apps can redirect module lookups to an in-bundle map without the source files existing on disk. Reuses the existing `__esModule` double-default and `importDefaultOnly` handling. State is stored on `globalThis.__EGG_BUNDLE_MODULE_LOADER__` so that bundled and external copies of @eggjs/utils share the same loader — required because a bundled entry and the external egg framework may resolve to different module instances. Required by the upcoming @eggjs/egg-bundler to bootstrap a bundled Egg app without filesystem discovery.
📝 WalkthroughWalkthroughThis PR introduces Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Deploying egg with
|
| Latest commit: |
48f44cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c2427fff.egg-cci.pages.dev |
| Branch Preview URL: | https://egg-15-typings.egg-cci.pages.dev |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5897 +/- ##
==========================================
- Coverage 85.52% 85.50% -0.02%
==========================================
Files 662 662
Lines 18863 18875 +12
Branches 3658 3662 +4
==========================================
+ Hits 16132 16140 +8
- Misses 2360 2362 +2
- Partials 371 373 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 shared package, @eggjs/typings, to house type-only contracts and global augmentations, starting with the BundleModuleLoader. The @eggjs/utils package has been updated to utilize this loader within importModule, enabling interception of module resolution for bundled applications. Furthermore, the PR includes refactored socket handling in cluster tests for better reliability and cross-platform line-ending normalization. I have no feedback to provide as there were no review comments to evaluate.
Deploying egg-v3 with
|
| Latest commit: |
48f44cb
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://2d6d5052.egg-v3.pages.dev |
| Branch Preview URL: | https://egg-15-typings.egg-v3.pages.dev |
There was a problem hiding this comment.
Pull request overview
Adds a new workspace package @eggjs/typings to centralize shared, cross-package type-only contracts for bundled module loading, and updates @eggjs/utils / @eggjs/core to consume those shared typings plus related repo documentation.
Changes:
- Introduce
packages/typingsexportingBundleModuleLoaderand theglobalThis.__EGG_BUNDLE_MODULE_LOADER__global augmentation. - Update
@eggjs/utilsto depend on@eggjs/typings, re-export the type, and keepimportModule()bundle-loader behavior + add/maintain tests. - Update
@eggjs/coreto include the shared ambient declarations via aglobal.d.tsshim, and refresh wiki/agent docs.
Reviewed changes
Copilot reviewed 15 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| wiki/packages/typings.md | New wiki page documenting the purpose/scope of @eggjs/typings. |
| wiki/log.md | Changelog entry for adding the shared typings package. |
| wiki/index.md | Adds Typings to the wiki package index. |
| tsconfig.json | Adds project reference for the new packages/typings. |
| packages/utils/test/bundle-import.test.ts | Adds Vitest coverage for bundle loader interception / normalization behavior. |
| packages/utils/test/snapshots/index.test.ts.snap | Updates exported-symbol snapshot to include setBundleModuleLoader. |
| packages/utils/src/import.ts | Switches to shared BundleModuleLoader type + re-export; bundle loader normalization/hook logic. |
| packages/utils/package.json | Adds @eggjs/typings workspace dependency. |
| packages/typings/tsdown.config.ts | Build entry for the new typings package. |
| packages/typings/tsconfig.json | Extends root TS config for the new package. |
| packages/typings/src/index.ts | Defines BundleModuleLoader and global augmentation. |
| packages/typings/package.json | Defines the new @eggjs/typings package metadata/exports. |
| packages/egg/test/cluster1/app_worker.test.ts | Makes raw HTTP response assertion resilient to CRLF differences; improves raw socket request settling. |
| packages/core/src/global.d.ts | Adds a thin ambient import shim for @eggjs/typings. |
| packages/core/package.json | Adds @eggjs/typings workspace dependency. |
| CLAUDE.md | Notes the new home for shared type-only contracts. |
| AGENTS.md | Notes packages/typings responsibility in repo structure. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/egg/test/cluster1/app_worker.test.ts (1)
143-176: LGTM —settle()gate cleanly prevents double settlement.The unified
settle(callback)helper correctly guardsend,error,close, andsetTimeoutagainst multiple resolution paths, clears the timer viasocket.setTimeout(0), and includes the partialresponsein the timeout error to aid debugging. Destroying the socket on timeout is the right call to free the FD promptly.One small optional thought: on the
'error'path you may also want tosocket.destroy()defensively — in practice Node'snetwill emitcloseaftererror, so this is purely belt-and-suspenders and not required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/cluster1/app_worker.test.ts` around lines 143 - 176, Add defensive socket.destroy() inside the rawRequest error handler: in the socket.on('error', (err) => ...) callback (inside the rawRequest function and alongside the existing call to settle), call socket.destroy() before invoking settle to ensure the file descriptor is freed promptly even if Node would later emit 'close'; keep the existing settle(...) logic and error rejection unchanged.CLAUDE.md (1)
9-9: Consider dropping this bullet from CLAUDE.md.This is general repo guidance (already captured in
AGENTS.mdline 12), not a Claude-specific import or override. Keeping it here duplicates content and growsCLAUDE.mdaway from being a thin pointer. As per coding guidelines, "Use CLAUDE.md only for Claude Code specific imports or overrides" and "Keep CLAUDE.md file thin and focused".♻️ Suggested removal
- Keep shared repository guidance in `AGENTS.md`. - Keep this file thin and use it only for Claude Code specific imports or overrides. -- Shared Egg type-only contracts live in `packages/typings` (`@eggjs/typings`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 9, Remove the duplicate guidance bullet in CLAUDE.md that states "Shared Egg type-only contracts live in `packages/typings` (`@eggjs/typings`)"—this content is general repo guidance already present in AGENTS.md and should not live in the Claude-specific overrides; edit CLAUDE.md to delete that bullet so the file remains thin and focused only on Claude-specific imports/overrides.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/package.json`:
- Line 46: The dependency '@eggjs/typings' must remain in
packages/core/package.json (it’s required at install time for the ambient import
in packages/core/src/global.d.ts: import '@eggjs/typings'; which is erased at
build time), but tsdown flags it as unused; update tsdown configs to whitelist
it by adding unused.ignore = ['@eggjs/typings'] in
packages/core/tsdown.config.ts and packages/utils/tsdown.config.ts (mirror the
pattern used in packages/koa/tsdown.config.ts and
packages/supertest/tsdown.config.ts) so the unused-dependency check no longer
fails while keeping the dependency in dependencies.
In `@packages/utils/package.json`:
- Around line 38-40: The build fails because `@eggjs/typings` is only used as a
TypeScript type (e.g., BundleModuleLoader referenced in src/import.ts) and
tsdown flags it as an unused runtime dependency; update tsdown config the same
way as for packages/core by adding `@eggjs/typings` to the unused-deps whitelist
in tsdown.config.ts for the utils package (or the shared config) so tsdown
ignores type-only imports from `@eggjs/typings` when checking packages/utils.
---
Nitpick comments:
In `@CLAUDE.md`:
- Line 9: Remove the duplicate guidance bullet in CLAUDE.md that states "Shared
Egg type-only contracts live in `packages/typings` (`@eggjs/typings`)"—this
content is general repo guidance already present in AGENTS.md and should not
live in the Claude-specific overrides; edit CLAUDE.md to delete that bullet so
the file remains thin and focused only on Claude-specific imports/overrides.
In `@packages/egg/test/cluster1/app_worker.test.ts`:
- Around line 143-176: Add defensive socket.destroy() inside the rawRequest
error handler: in the socket.on('error', (err) => ...) callback (inside the
rawRequest function and alongside the existing call to settle), call
socket.destroy() before invoking settle to ensure the file descriptor is freed
promptly even if Node would later emit 'close'; keep the existing settle(...)
logic and error rejection unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ff7c241-00fb-4829-b86d-736346ca1f84
⛔ Files ignored due to path filters (1)
packages/utils/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (16)
AGENTS.mdCLAUDE.mdpackages/core/package.jsonpackages/core/src/global.d.tspackages/egg/test/cluster1/app_worker.test.tspackages/typings/package.jsonpackages/typings/src/index.tspackages/typings/tsconfig.jsonpackages/typings/tsdown.config.tspackages/utils/package.jsonpackages/utils/src/import.tspackages/utils/test/bundle-import.test.tstsconfig.jsonwiki/index.mdwiki/log.mdwiki/packages/typings.md
| "dependencies": { | ||
| "@eggjs/typings": "workspace:*" | ||
| }, |
There was a problem hiding this comment.
Build failure here likely shares the root cause flagged on packages/core/package.json.
BundleModuleLoader is consumed from @eggjs/typings only as a type in src/import.ts (and erased at build time under verbatimModuleSyntax), so tsdown's unused-dep check will flag it the same way it does for @eggjs/core. Apply the same tsdown.config.ts whitelist fix here. See the consolidated comment on packages/core/package.json for details.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/utils/package.json` around lines 38 - 40, The build fails because
`@eggjs/typings` is only used as a TypeScript type (e.g., BundleModuleLoader
referenced in src/import.ts) and tsdown flags it as an unused runtime
dependency; update tsdown config the same way as for packages/core by adding
`@eggjs/typings` to the unused-deps whitelist in tsdown.config.ts for the utils
package (or the shared config) so tsdown ignores type-only imports from
`@eggjs/typings` when checking packages/utils.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/egg/test/cluster1/app_worker.test.ts (1)
44-44: Optional: consider asserting on the raw body too.Normalizing CRLF→LF on both sides makes the assertion resilient to line-ending differences in the wire response, which is the intent. If the goal is also to lock down the on-the-wire format (the server response in
packages/egg/src/lib/application.tsis built with explicit\r\nseparators in headers but the HTML template uses\n), an additionalassert.equal(body, DEFAULT_BAD_REQUEST_HTML)(without normalization) would catch accidental line-ending regressions in the body. Non-blocking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/egg/test/cluster1/app_worker.test.ts` at line 44, Add an extra assertion to verify the raw on-the-wire response body in addition to the normalized check: after the existing assert.equal(body.replaceAll('\r\n', '\n'), DEFAULT_BAD_REQUEST_HTML.replaceAll('\r\n', '\n')), add a second assertion asserting body === DEFAULT_BAD_REQUEST_HTML (i.e. assert.equal(body, DEFAULT_BAD_REQUEST_HTML)) so accidental CRLF/LF regressions in the produced response are caught; locate this in app_worker.test.ts around the existing assert.equal call that references body and DEFAULT_BAD_REQUEST_HTML.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/egg/test/cluster1/app_worker.test.ts`:
- Line 44: Add an extra assertion to verify the raw on-the-wire response body in
addition to the normalized check: after the existing
assert.equal(body.replaceAll('\r\n', '\n'),
DEFAULT_BAD_REQUEST_HTML.replaceAll('\r\n', '\n')), add a second assertion
asserting body === DEFAULT_BAD_REQUEST_HTML (i.e. assert.equal(body,
DEFAULT_BAD_REQUEST_HTML)) so accidental CRLF/LF regressions in the produced
response are caught; locate this in app_worker.test.ts around the existing
assert.equal call that references body and DEFAULT_BAD_REQUEST_HTML.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 243654d3-7163-421f-bf28-cdd36a4276c1
📒 Files selected for processing (3)
packages/core/tsdown.config.tspackages/egg/test/cluster1/app_worker.test.tspackages/utils/tsdown.config.ts
✅ Files skipped from review due to trivial changes (2)
- packages/core/tsdown.config.ts
- packages/utils/tsdown.config.ts
Summary
Adds a new
@eggjs/typingsworkspace package to own shared bundle module loader typing, includingBundleModuleLoaderand theglobalThis.__EGG_BUNDLE_MODULE_LOADER__augmentation.This lets
@eggjs/utilsconsume the shared type package instead of owning a local global view or depending on@eggjs/coretypings, while@eggjs/corekeeps itsglobal.d.tsas a thin import of the shared package.Also updates
AGENTS.md,CLAUDE.md, and the repo wiki for the new shared type-only package responsibility.Test plan
pnpm installpnpm --filter=@eggjs/typings run typecheckpnpm --filter=@eggjs/utils run typecheckpnpm --filter=@eggjs/core run typecheckpnpm exec vitest run packages/utils/test/bundle-import.test.ts --bail 1 --retry 0pnpm -r --if-present run typecheckpnpm exec oxlint --type-aware --type-check --quiet(0 errors, existing warnings only)pnpm exec oxfmt --check AGENTS.md CLAUDE.md tsconfig.json packages/core/package.json packages/core/src/global.d.ts packages/utils/package.json packages/utils/src/import.ts packages/typings/package.json packages/typings/tsconfig.json packages/typings/tsdown.config.ts packages/typings/src/index.ts wiki/index.md wiki/log.md wiki/packages/typings.mdpnpm --filter=@eggjs/typings exec tsdownpnpm --filter=@eggjs/utils exec tsdownpnpm --filter=@eggjs/core exec tsdownpnpm exec vitest run packages/egg/test/cluster1/app_worker.test.ts --bail 1 --retry 0Note: root
pnpm run typecheckstill exits before typechecking atut run cleanwithERROR Script 'clean' not found in package.json, same local utoo issue observed in PR #5867. The direct recursive workspace typecheck above passed.Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
Chores