feat: add @eggjs/typings package#5896
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a new Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## next #5896 +/- ##
=======================================
Coverage 85.52% 85.52%
=======================================
Files 662 662
Lines 18863 18863
Branches 3658 3658
=======================================
Hits 16132 16132
Misses 2360 2360
Partials 371 371 ☔ 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 @eggjs/typings package and implements a bundle module loader in @eggjs/utils to allow intercepting module imports via a global loader. The importModule function was updated to support this loader, including logic for path normalization and handling transpiled module shapes. Review feedback suggests refining the module unwrapping logic to include function types and ensure safer property access when handling 'double default' exports.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/utils/src/import.ts (2)
414-423: Optional: factor the export-unwrap logic into a helper.The unwrap pattern here is the third copy in this file — almost identical to the snapshot-loader branch (lines 428-436) and the ESM branch (lines 453-485), differing only in defensive
typeof obj === 'object'checks. A small private helper, e.g.unwrapModule(obj, importDefaultOnly), would keep all three branches in lockstep so a future fix to the interop heuristic doesn't have to be made in three places.Not blocking — current behavior is consistent with the test matrix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/import.ts` around lines 414 - 423, Extract the repeated export-unwrapping logic into a small private helper function (e.g. unwrapModule(obj, importDefaultOnly)) and replace the three in-place copies (the one inside the _bundleModuleLoader branch that calls normalizeBundleModulePath(filepath) and checks options?.importDefaultOnly, the snapshot-loader branch, and the ESM branch) with calls to that helper; the helper should perform the same checks currently present (detect obj?.default?.__esModule === true && 'default' in obj, and when importDefaultOnly and obj is an object return obj.default) so behavior remains identical while centralizing the heuristic for future fixes.
409-411: Minor:setBundleModuleLoader(undefined)leaves the property defined onglobalThis.Assigning
undefinedkeeps__EGG_BUNDLE_MODULE_LOADER__as an own property ofglobalThis(with valueundefined). The runtime check on line 415 still treats it as falsy, so behavior is correct. Considerdelete globalThis.__EGG_BUNDLE_MODULE_LOADER__whenloader === undefinedif you want a clean tear-down (e.g. for tests, snapshot inspectors). Purely cosmetic.♻️ Optional cleanup
export function setBundleModuleLoader(loader: BundleModuleLoader | undefined): void { - globalThis.__EGG_BUNDLE_MODULE_LOADER__ = loader; + if (loader === undefined) { + delete globalThis.__EGG_BUNDLE_MODULE_LOADER__; + } else { + globalThis.__EGG_BUNDLE_MODULE_LOADER__ = loader; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/import.ts` around lines 409 - 411, The setter setBundleModuleLoader currently assigns undefined to globalThis.__EGG_BUNDLE_MODULE_LOADER__, leaving the property present; change setBundleModuleLoader to delete globalThis.__EGG_BUNDLE_MODULE_LOADER__ when loader === undefined and only assign the loader value when it's not undefined so tests/snapshots see a clean teardown; reference function name setBundleModuleLoader and property __EGG_BUNDLE_MODULE_LOADER__.
🤖 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/utils/src/import.ts`:
- Around line 414-423: Extract the repeated export-unwrapping logic into a small
private helper function (e.g. unwrapModule(obj, importDefaultOnly)) and replace
the three in-place copies (the one inside the _bundleModuleLoader branch that
calls normalizeBundleModulePath(filepath) and checks options?.importDefaultOnly,
the snapshot-loader branch, and the ESM branch) with calls to that helper; the
helper should perform the same checks currently present (detect
obj?.default?.__esModule === true && 'default' in obj, and when
importDefaultOnly and obj is an object return obj.default) so behavior remains
identical while centralizing the heuristic for future fixes.
- Around line 409-411: The setter setBundleModuleLoader currently assigns
undefined to globalThis.__EGG_BUNDLE_MODULE_LOADER__, leaving the property
present; change setBundleModuleLoader to delete
globalThis.__EGG_BUNDLE_MODULE_LOADER__ when loader === undefined and only
assign the loader value when it's not undefined so tests/snapshots see a clean
teardown; reference function name setBundleModuleLoader and property
__EGG_BUNDLE_MODULE_LOADER__.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: af43da8f-b4c4-4d8f-9d0b-c8ad88f37c1b
⛔ Files ignored due to path filters (1)
packages/utils/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/core/package.jsonpackages/core/src/index.tspackages/typings/package.jsonpackages/typings/src/global.tspackages/typings/src/index.tspackages/typings/tsconfig.jsonpackages/typings/tsdown.config.tspackages/utils/package.jsonpackages/utils/src/import.tspackages/utils/test/bundle-import.test.tstsconfig.json
There was a problem hiding this comment.
Pull request overview
Adds a new shared workspace package @eggjs/typings to centralize cross-package types and a global augmentation entry, then wires it into @eggjs/utils and @eggjs/core to support a global bundle module loader hook.
Changes:
- Add new
@eggjs/typingspackage withBundleModuleLoadertype + global augmentation entry (@eggjs/typings/global). - Update
@eggjs/utilsimport utilities to use the shared typings and addsetBundleModuleLoader()+ bundle-loader interception logic inimportModule(). - Ensure
@eggjs/coreloads the shared global augmentation and add test coverage + snapshot updates for the new utils export.
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Adds packages/typings as a TS project reference. |
| packages/utils/src/import.ts | Introduces bundle loader global hook, consumes @eggjs/typings, and intercepts importModule(). |
| packages/utils/package.json | Adds @eggjs/typings workspace dependency. |
| packages/utils/test/bundle-import.test.ts | Adds coverage for bundle loader behavior (fallback, default-only, virtual specifiers, Windows path normalization). |
| packages/utils/test/snapshots/index.test.ts.snap | Updates export snapshot to include setBundleModuleLoader. |
| packages/typings/package.json | New package metadata/exports (including ./global subpath). |
| packages/typings/src/index.ts | Defines BundleModuleLoader type. |
| packages/typings/src/global.ts | Declares globalThis.__EGG_BUNDLE_MODULE_LOADER__ augmentation. |
| packages/typings/tsconfig.json | TS config extending repo root. |
| packages/typings/tsdown.config.ts | Build entries for index and global. |
| packages/core/src/index.ts | Loads @eggjs/typings/global to ensure augmentation is applied. |
| packages/core/package.json | Adds @eggjs/typings workspace dependency. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/utils/src/import.ts (1)
7-9: Optional: collapse into a single re-export.
import type { BundleModuleLoader }is only used as a parameter type annotation onsetBundleModuleLoader(Line 438), which under TypeScript'sisolatedModules/erasable-syntax rules does not require a separate import — a singleexport typere-export is enough since you can reference re-exported types within the same file when using--isolatedModules. If you'd rather keep both for clarity, leave as-is.♻️ Proposed simplification
import '@eggjs/typings/global'; -import type { BundleModuleLoader } from '@eggjs/typings'; -export type { BundleModuleLoader } from '@eggjs/typings'; +import type { BundleModuleLoader } from '@eggjs/typings'; +export type { BundleModuleLoader };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/utils/src/import.ts` around lines 7 - 9, Collapse the two separate imports into a single type re-export: keep the side-effect import "import '@eggjs/typings/global';" but remove the separate "import type { BundleModuleLoader }" and rely on "export type { BundleModuleLoader }" only; this lets references like the parameter type on setBundleModuleLoader use the re-exported BundleModuleLoader under isolatedModules/erasable-syntax rules while preserving the global typings import.
🤖 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/utils/src/import.ts`:
- Around line 7-9: Collapse the two separate imports into a single type
re-export: keep the side-effect import "import '@eggjs/typings/global';" but
remove the separate "import type { BundleModuleLoader }" and rely on "export
type { BundleModuleLoader }" only; this lets references like the parameter type
on setBundleModuleLoader use the re-exported BundleModuleLoader under
isolatedModules/erasable-syntax rules while preserving the global typings
import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b4f4f25c-9082-4fd0-bc7b-a5621384de2b
⛔ Files ignored due to path filters (1)
packages/utils/test/__snapshots__/index.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (11)
packages/core/package.jsonpackages/core/src/index.tspackages/typings/package.jsonpackages/typings/src/global.tspackages/typings/src/index.tspackages/typings/tsconfig.jsonpackages/typings/tsdown.config.tspackages/utils/package.jsonpackages/utils/src/import.tspackages/utils/test/bundle-import.test.tstsconfig.json
✅ Files skipped from review due to trivial changes (9)
- packages/core/src/index.ts
- packages/typings/src/index.ts
- tsconfig.json
- packages/core/package.json
- packages/typings/tsconfig.json
- packages/typings/tsdown.config.ts
- packages/typings/src/global.ts
- packages/utils/package.json
- packages/typings/package.json
关联 EGG-15。
Summary:
Local checks:
Note: pnpm run typecheck still exits before workspace checks because ut run clean-dist calls clean and reports ERROR Script 'clean' not found in package.json, so the direct workspace typecheck command above was used for workspace verification.
Summary by CodeRabbit
New Features
Documentation