test(utils): formalize bundle/snapshot module-loader hooks#6002
Merged
Conversation
Document and add regression coverage for the `__EGG_BUNDLE_MODULE_LOADER__` → `__EGG_MODULE_IMPORTER__` → native priority used by bundle/snapshot mode. - cover the V8 snapshot-restore path where `__EGG_MODULE_IMPORTER__ = require` loads ESM with no dynamic-import callback (inline sync-require test plus a spawned `node:vm` fixture that asserts the no-callback premise) - expand JSDoc on `BundleModuleLoader` / `ModuleImporter` to spell out the contract for egg-bundler generated entries - document the hook priority in the @eggjs/utils README No load-semantics change; types/declarations already existed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR expands module-loader hook documentation and typings, adds a ChangesModule-loader hook contract
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 #6002 +/- ##
=======================================
Coverage 84.88% 84.88%
=======================================
Files 674 674
Lines 20269 20269
Branches 4039 4039
=======================================
Hits 17205 17205
Misses 2633 2633
Partials 431 431 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![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 formalizes and documents the priority and behavior of the bundle and snapshot module-loading hooks, including EGG_BUNDLE_MODULE_LOADER, the snapshot loader, and EGG_MODULE_IMPORTER. It also adds test coverage for loading ESM modules synchronously via require() to simulate V8 startup-snapshot restore environments. The reviewer pointed out that the new tests rely on experimental Node.js features (such as require() of ESM and --experimental-strip-types) only available in Node.js >= 22, which could cause CI failures on older Node versions. They suggested conditionally skipping these tests based on the running Node.js version.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+59
to
+85
| it('loads a real ESM module through a synchronous require-based importer', async () => { | ||
| // The importer return value is awaited, so a synchronous `require()` (which | ||
| // returns the module synchronously) is a valid importer. require() can load | ||
| // ESM on Node >= 22, which is what the snapshot entry relies on. | ||
| const require = createRequire(import.meta.url); | ||
| let calls = 0; | ||
| globalThis.__EGG_MODULE_IMPORTER__ = ((filepath: string) => { | ||
| calls++; | ||
| return require(filepath); | ||
| }) as typeof globalThis.__EGG_MODULE_IMPORTER__; | ||
|
|
||
| const result = await importModule(getFilepath('esm')); | ||
| assert.equal(calls, 1); | ||
| assert.equal(result.one, 1); | ||
| assert.deepEqual(result.default, { foo: 'bar' }); | ||
| }); | ||
|
|
||
| it('uses a require-based importer when no dynamic import callback exists', async () => { | ||
| // Reproduces the V8 snapshot-restore environment in a child process: native | ||
| // import() has no host callback, so importModule() must route ESM loading | ||
| // through the require-based __EGG_MODULE_IMPORTER__. See the fixture. | ||
| await coffee | ||
| .spawn(process.execPath, ['--experimental-strip-types', getFilepath('module-importer-require-esm/run.mjs')]) | ||
| .expect('stdout', /IMPORTER_REQUIRE_ESM_OK/) | ||
| .expect('code', 0) | ||
| .end(); | ||
| }); |
Contributor
There was a problem hiding this comment.
These tests rely on experimental Node.js features that are not supported or enabled by default on all Node.js versions:
- require() of ESM modules (used in both tests) is only supported on Node.js >= 22.0.0, and is only enabled by default starting from Node.js v22.12.0. On Node.js versions between v22.0.0 and v22.11.0, it requires the --experimental-require-module flag. On older Node.js versions (v18/v20), it is not supported at all.
- --experimental-strip-types (used in the second test) was introduced in Node.js v22.6.0.
To prevent the test suite from failing when executed on older Node.js versions (e.g., in CI environments running Node 18 or 20), we should:
- Dynamically detect support for these features and conditionally skip the tests if they are not supported/enabled.
- Explicitly pass the --experimental-require-module flag to the spawned child process to ensure it works on Node.js versions between v22.6.0 and v22.11.0.
const [major, minor] = process.versions.node.split('.').map(Number);
const supportsRequireEsm =
major > 22 ||
(major === 22 && minor >= 12) ||
process.execArgv.includes('--experimental-require-module');
const supportsStripTypes = major > 22 || (major === 22 && minor >= 6);
(supportsRequireEsm ? it : it.skip)('loads a real ESM module through a synchronous require-based importer', async () => {
// The importer return value is awaited, so a synchronous require() (which
// returns the module synchronously) is a valid importer. require() can load
// ESM on Node >= 22, which is what the snapshot entry relies on.
const require = createRequire(import.meta.url);
let calls = 0;
globalThis.__EGG_MODULE_IMPORTER__ = ((filepath: string) => {
calls++;
return require(filepath);
}) as typeof globalThis.__EGG_MODULE_IMPORTER__;
const result = await importModule(getFilepath('esm'));
assert.equal(calls, 1);
assert.equal(result.one, 1);
assert.deepEqual(result.default, { foo: 'bar' });
});
(supportsStripTypes ? it : it.skip)('uses a require-based importer when no dynamic import callback exists', async () => {
// Reproduces the V8 snapshot-restore environment in a child process: native
// import() has no host callback, so importModule() must route ESM loading
// through the require-based __EGG_MODULE_IMPORTER__. See the fixture.
await coffee
.spawn(process.execPath, [
'--experimental-strip-types',
'--experimental-require-module',
getFilepath('module-importer-require-esm/run.mjs'),
])
.expect('stdout', /IMPORTER_REQUIRE_ESM_OK/)
.expect('code', 0)
.end();
});
Contributor
Author
There was a problem hiding this comment.
Thanks for the careful read. This is intentional and safe for this repo's supported baseline, so I'm not adding the version guards:
- Node baseline. Both the root and
@eggjs/utilspackage.jsondeclareengines.node: ">=22.18.0", and the CI matrix runs only Node 22 and 24 (.github/workflows/ci.yml). Node 18/20 and 22.x < 22.12 are not supported targets, so the tests never run there. require(ESM)is enabled by default since Node 22.12 — well below the>=22.18.0floor — so no--experimental-require-moduleflag is needed.- Type stripping is on by default since Node 22.18, which is exactly the
enginesfloor. The explicit--experimental-strip-typesflag is harmless on 22.18+ and matches the existing precedent in the same package:packages/utils/test/bundle-import.test.tsalready spawns fixtures with--experimental-strip-typesand no version guards.
Adding it.skip guards only here would be inconsistent with that sibling test and would mask a real failure if the supported baseline ever regressed. If the repo later lowers engines below 22.12, the guards (and the spawned-process flag) would be the right move — but that's a repo-wide policy change, out of scope for this PR.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/typings/src/index.ts`:
- Line 55: The ModuleImporter type is too narrow because it only allows async
importers, while the contract also supports sync require-style importers. Update
the ModuleImporter alias in the typings entrypoint to accept both synchronous
and asynchronous return values, using the ModuleImporter symbol so callers can
pass either form without casting.
🪄 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: 71f5f357-8ab2-441d-97ed-731c75fada99
📒 Files selected for processing (6)
packages/typings/src/index.tspackages/utils/README.mdpackages/utils/test/fixtures/module-importer-require-esm/run.mjspackages/utils/test/module-importer.test.tswiki/log.mdwiki/packages/utils.md
| * importer (`createRequire()` over the bundle output dir); `require()` can | ||
| * load ESM on Node >= 22, so modules resolve without dynamic import. | ||
| */ | ||
| export type ModuleImporter = (filePath: string) => Promise<unknown>; |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=ts -C3 '__EGG_MODULE_IMPORTER__'
ast-grep run --pattern '$X.__EGG_MODULE_IMPORTER__($_)' --lang typescriptRepository: eggjs/egg
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- packages/typings/src/index.ts (relevant section) ---'
sed -n '1,120p' packages/typings/src/index.ts
echo
echo '--- search ModuleImporter / __EGG_MODULE_IMPORTER__ ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'ModuleImporter|__EGG_MODULE_IMPORTER__' packages
echo
echo '--- search related importer usage in packages/utils and packages/typings ---'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'module[- ]importer|importer|require\(' packages/utils packages/typingsRepository: eggjs/egg
Length of output: 10598
Widen ModuleImporter to accept sync returns
packages/typings/src/index.ts:55 still forces Promise<unknown>, but the contract already allows synchronous require()-style importers. Change it to unknown | Promise<unknown> so callers don’t need a cast.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/typings/src/index.ts` at line 55, The ModuleImporter type is too
narrow because it only allows async importers, while the contract also supports
sync require-style importers. Update the ModuleImporter alias in the typings
entrypoint to accept both synchronous and asynchronous return values, using the
ModuleImporter symbol so callers can pass either form without casting.
killagu
added a commit
that referenced
this pull request
Jun 28, 2026
…nd cnpmcore e2e (#6003) ## Motivation Restoring a V8 startup snapshot requires **Node.js >= 24**: Node.js 22 aborts during deserialization of a non-trivial Egg heap with the native fatal `Check failed: current == end_slot_index` (a V8 bug). Building a snapshot still works on Node.js >= 22. Today nothing enforces or documents this, and there is no regression coverage. This PR adds a runtime gate, documentation, and an e2e regression — without changing the snapshot mechanism itself. Builds on the snapshot work already on `next` (#5998 entry/prelude + `egg-bin snapshot` command, #5999 lazy-external network stack, #6001 logger reopen, #6002 module-loader hooks). ## Scope **Runtime gate (restore ≥ 24; build stays ≥ 22)** - `@eggjs/scripts`: `egg-scripts start --snapshot-blob` refuses to launch on Node.js < 24 with a clear error *before* spawning, checking the major version of the resolved `--node` target binary (not just the egg-scripts runtime). Also adds `allowNo: true` to the `sourcemap` flag so `--no-sourcemap` is accepted. - `@eggjs/egg-bundler`: a defense-in-depth guard in the generated deserialize-main for direct `node --snapshot-blob` launches that manage to deserialize on an unsupported runtime. - `@eggjs/bin`: `snapshot build` prints a note that restoring needs Node.js >= 24. **Docs** - Enrich `site/docs/advanced/snapshot.md` (EN + ZH): Node version requirements, the CLI workflow (`egg-bin snapshot build` → `egg-scripts start --snapshot-blob`), how it works (load module graph → run to `configWillLoad` → freeze; restore = `didReady` + listen), performance (~233ms vs ~942ms, ~4× on cnpmcore), and known limitations. - Wire the page into the VitePress sidebar (EN + ZH) — it existed but was unreachable. **CI** - Add a blocking `cnpmcore-snapshot` ecosystem-ci e2e (Node 24): snapshot build → restore via `egg-scripts start --snapshot-blob` → `curl /-/ping` == 200 → stop. Wires `repo.json`, `patch-project.ts`, `.gitignore`. - Extract the shared health-check poll into `ecosystem-ci/wait-health.sh`, used by both the `cnpmcore` and `cnpmcore-snapshot` jobs. ## Test evidence - `pnpm --filter=@eggjs/scripts --filter=@eggjs/egg-bundler --filter=@eggjs/bin run typecheck` — clean. - `@eggjs/scripts` `snapshot-start.test.ts` (4 tests, incl. a Node<24 gate test and a `--no-sourcemap` parse regression test) + `start-unit.test.ts` — pass. - `@eggjs/bin` `snapshot.test.ts` — pass. - `@eggjs/egg-bundler` `EntryGenerator` canonical snapshot regenerated for the new guard; the rest of the suite matches the pre-change baseline (a few pre-existing macOS/Node-22 path-resolution failures are unrelated). - A multi-agent diff review was run and all confirmed findings addressed (most notably: `--no-sourcemap` is now parseable via `allowNo: true` — without it the e2e job would have failed 100%). ## Notes - The `cnpmcore-snapshot` job is correct-by-construction but **could not be validated on the author's machine** (Node 22, no MySQL+cnpmcore build); it relies on the supported path (lazy-external from #5999, no manual stubs) and is validated by this PR's CI run. - The job is intentionally a **separate matrix project** (not folded into the existing cnpmcore job) for failure isolation. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added “V8 Startup Snapshot” documentation navigation. * Added support for snapshot project configuration (shared project-root patching). * **Bug Fixes** * Enforced Node.js version gating for V8 snapshot restore (requires Node.js ≥ 24) and improved snapshot restore safety. * Improved snapshot lazy-external behavior, including correct external named-export handling. * Updated the start command to allow `--no-sourcemap`. * **Documentation** * Expanded snapshot docs with requirements, workflow details, performance, and limitations. * **Tests** * Updated snapshot start/version-gating and lazy-external readiness assertions. * **Chores** * Improved E2E readiness checks with consistent polling, timeouts, and error log output. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.8 (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
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.
Motivation
Bundle / snapshot mode loads modules through two
globalThisextension points:__EGG_BUNDLE_MODULE_LOADER__— the inlined bundle map (set viasetBundleModuleLoader()).__EGG_MODULE_IMPORTER__— an injectable importer that replaces nativeawait import().The second one is load-bearing for V8 startup-snapshot restore: the deserialized main function runs with no host dynamic-import callback, so
import()throws (ERR_VM_DYNAMIC_IMPORT_CALLBACK_MISSING). The snapshot entry generated byegg-bundlerinstalls a synchronousrequire()-based importer (require()can load ESM on Node >= 22), so modules resolve without dynamic import.These hooks already existed (types in
@eggjs/typings,setBundleModuleLoader+ basic importer tests) but the snapshot-restore path was untested and the contract was undocumented. This PR formalizes them as a documented, tested extension point — no load-semantics change.Scope
packages/utils): add coverage for the snapshot-restore path where__EGG_MODULE_IMPORTER__ = requireloads a real ESM module — an inline synchronous require-based importer plus a spawnednode:vmfixture that first asserts the no-dynamic-import-callback premise, then provesimportModule()routes through the require importer and bypasses nativeimport().BundleModuleLoader/ModuleImporter,@eggjs/utilsREADME, wiki): document the full resolution order —__EGG_BUNDLE_MODULE_LOADER__→ snapshot loader (setSnapshotModuleLoader) →__EGG_MODULE_IMPORTER__→ native — including which path each caller passes (@eggjs/utilspasses the un-normalizedimportResolve()result; the tegg loader passes the original filepath POSIX-normalized) and that@eggjs/core'sManifestLoaderFSconsults only the bundle loader directly (importer/native via the@eggjs/loader-fsfallback).No production code changed —
packages/utils/src/import.tsand the loaders are untouched.Test evidence
pnpm --filter=@eggjs/utils exec vitest run— 77 passed, 13 skipped.packages/coremanifest_loader_fs.test.ts(existing bundle-mode routing coverage) — 7 passed.typecheck(utils + typings),oxlint --type-aware --type-check,oxfmt --check— all clean.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation