-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
feat(utils): add setBundleModuleLoader runtime hook #5867
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: next
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -394,7 +394,35 @@ export function setSnapshotModuleLoader(loader: SnapshotModuleLoader): void { | |
| isESM = false; | ||
| } | ||
|
|
||
| /** | ||
| * Module loader for bundled egg apps. Called with the raw `importModule()` | ||
| * filepath (posix-normalized) before `importResolve`, so bundled apps can | ||
| * serve modules that no longer exist on disk. Return `undefined` to fall | ||
| * through to the standard import path. | ||
| */ | ||
| export type BundleModuleLoader = (filepath: string) => unknown; | ||
|
|
||
| /** | ||
| * Register a bundle module loader. Uses globalThis so that bundled and | ||
| * external copies of @eggjs/utils share the same loader. | ||
| */ | ||
| export function setBundleModuleLoader(loader: BundleModuleLoader | undefined): void { | ||
| (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ = loader; | ||
| if (loader) isESM = false; | ||
| } | ||
|
Comment on lines
+409
to
+412
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify there is no unregister branch restoring isESM today.
rg -n -A12 -B2 'export function setBundleModuleLoader' packages/utils/src/import.ts
rg -n 'isESM\s*=' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 1118 🏁 Script executed: # Find all calls to setBundleModuleLoader to understand usage pattern
rg -n 'setBundleModuleLoader' --type ts --type jsRepository: eggjs/egg Length of output: 876 🏁 Script executed: # Check what _snapshotModuleLoader is and where it's defined
rg -n '_snapshotModuleLoader' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 247 🏁 Script executed: # Look at more context around the isESM initialization and snapshot logic
sed -n '1,100p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 4089 🏁 Script executed: # Examine the test file to understand the impact of isESM state
cat -n packages/utils/test/bundle-import.test.tsRepository: eggjs/egg Length of output: 2702 🏁 Script executed: # Check if isESM is used in importModule logic to understand the actual impact
sed -n '414,450p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1728 🏁 Script executed: # Check if there are any test failures related to isESM state between tests
cd packages/utils && npm test 2>&1 | head -100Repository: eggjs/egg Length of output: 273 🏁 Script executed: # Look at how snapshot loader is set to understand the state management pattern
sed -n '375,405p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1465 🏁 Script executed: # Check if there's any cleanup/reset of isESM in test setup/teardown
rg -n 'isESM|beforeEach|afterEach|before|after' packages/utils/test/bundle-import.test.tsRepository: eggjs/egg Length of output: 126 🏁 Script executed: # Check setSnapshotModuleLoader for similar isESM restoration issue
sed -n '388,402p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 705 🏁 Script executed: # Verify all `any` casts in this file to understand full scope
rg -n 'as any' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 265 🏁 Script executed: # Verify that _snapshotModuleLoader is in scope and accessible where proposed fix would apply
sed -n '375,430p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 2817 🏁 Script executed: # Check if there are any type definitions for globalThis to use instead of 'as any'
rg -n 'globalThis|declare' packages/utils/src/import.ts | head -20Repository: eggjs/egg Length of output: 309 Restore Line 411 only sets Additionally, replace Lines 410 and 415 use 🛠️ Proposed fix for state restoration+const detectedIsESM = isESM;
+
export function setBundleModuleLoader(loader: BundleModuleLoader | undefined): void {
(globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ = loader;
- if (loader) isESM = false;
+ if (loader) {
+ isESM = false;
+ } else if (!_snapshotModuleLoader) {
+ isESM = detectedIsESM;
+ }
}🤖 Prompt for AI Agents |
||
|
|
||
| export async function importModule(filepath: string, options?: ImportModuleOptions): Promise<any> { | ||
| const _bundleModuleLoader: BundleModuleLoader | undefined = (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__; | ||
|
Comment on lines
+410
to
+415
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Verify current any-casts on bundle loader global access.
rg -n '\(globalThis as any\)\.__EGG_BUNDLE_MODULE_LOADER__' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 233 🏁 Script executed: #!/bin/bash
# Find all occurrences of __EGG_BUNDLE_MODULE_LOADER__ in the codebase
rg -n '__EGG_BUNDLE_MODULE_LOADER__' --type ts --type tsxRepository: eggjs/egg Length of output: 80 🏁 Script executed: #!/bin/bash
# Show the context around the current usage in import.ts (lines 400-420)
sed -n '400,420p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1158 🏁 Script executed: #!/bin/bash
# Check the beginning of import.ts to see current exports and type definitions
head -50 packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1968 🏁 Script executed: #!/bin/bash
# Search all files for __EGG_BUNDLE_MODULE_LOADER__ without file type filter
rg -n '__EGG_BUNDLE_MODULE_LOADER__'Repository: eggjs/egg Length of output: 291 🏁 Script executed: #!/bin/bash
# Get the exact line numbers from the import.ts file (use wc to check file length first)
wc -l packages/utils/src/import.tsRepository: eggjs/egg Length of output: 85 🏁 Script executed: #!/bin/bash
# Get lines around 410-415 from import.ts
sed -n '405,420p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 940 🏁 Script executed: #!/bin/bash
# Get context around the BundleModuleLoader type definition (should be earlier in file)
sed -n '390,425p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1762 🏁 Script executed: #!/bin/bash
# Search for any existing declare global blocks in the file
rg -n 'declare global' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 35 🏁 Script executed: #!/bin/bash
# Get the section with BundleModuleLoader export (likely around line 400 or earlier)
rg -B 5 -A 2 'export type BundleModuleLoader' packages/utils/src/import.tsRepository: eggjs/egg Length of output: 390 🏁 Script executed: #!/bin/bash
# Double-check for any usages in plugins directory
rg -n '__EGG_BUNDLE_MODULE_LOADER__' plugins/ 2>/dev/null || echo "No matches in plugins/"Repository: eggjs/egg Length of output: 75 🏁 Script executed: #!/bin/bash
# Verify the exact structure - show where BundleModuleLoader export appears
sed -n '400,420p' packages/utils/src/import.ts | cat -nRepository: eggjs/egg Length of output: 1158 Replace Lines 410 and 415 use ♻️ Proposed refactor export type BundleModuleLoader = (filepath: string) => unknown;
+declare global {
+ // shared runtime hook across duplicated package instances
+ var __EGG_BUNDLE_MODULE_LOADER__: BundleModuleLoader | undefined;
+}
+
/**
* Register a bundle module loader. Uses globalThis so that bundled and
* external copies of `@eggjs/utils` share the same loader.
*/
export function setBundleModuleLoader(loader: BundleModuleLoader | undefined): void {
- (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ = loader;
+ globalThis.__EGG_BUNDLE_MODULE_LOADER__ = loader;
if (loader) isESM = false;
}
export async function importModule(filepath: string, options?: ImportModuleOptions): Promise<any> {
- const _bundleModuleLoader: BundleModuleLoader | undefined = (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__;
+ const _bundleModuleLoader = globalThis.__EGG_BUNDLE_MODULE_LOADER__;🤖 Prompt for AI Agents |
||
| if (_bundleModuleLoader) { | ||
| const hit = _bundleModuleLoader(filepath.replaceAll('\\', '/')); | ||
|
Comment on lines
+415
to
+417
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is a consistency issue when multiple instances of const _bundleModuleLoader: BundleModuleLoader | undefined = (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__;
if (_bundleModuleLoader) {
isESM = false;
const hit = _bundleModuleLoader(filepath.replaceAll('\\', '/')); |
||
| if (hit !== undefined) { | ||
| let obj = hit as any; | ||
| if (obj?.default?.__esModule === true && 'default' in obj.default) obj = obj.default; | ||
| if (options?.importDefaultOnly && obj && typeof obj === 'object' && 'default' in obj) obj = obj.default; | ||
| return obj; | ||
|
Comment on lines
+409
to
+422
|
||
| } | ||
| } | ||
|
|
||
| const moduleFilePath = importResolve(filepath, options); | ||
|
|
||
| if (_snapshotModuleLoader) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| import { strict as assert } from 'node:assert'; | ||
|
|
||
| import { afterEach, describe, it } from 'vitest'; | ||
|
|
||
| import { importModule, setBundleModuleLoader } from '../src/import.ts'; | ||
| import { getFilepath } from './helper.ts'; | ||
|
|
||
| describe('test/bundle-import.test.ts', () => { | ||
| afterEach(() => { | ||
| setBundleModuleLoader(undefined); | ||
| }); | ||
|
|
||
| it('returns the real module when no bundle loader is registered', async () => { | ||
| const result = await importModule(getFilepath('esm')); | ||
| assert.ok(result); | ||
| assert.equal(typeof result, 'object'); | ||
| }); | ||
|
|
||
| it('intercepts importModule with the registered loader', async () => { | ||
| const seen: string[] = []; | ||
| const fakeModule = { default: { hello: 'bundle' }, other: 'stuff' }; | ||
| setBundleModuleLoader((p) => { | ||
| seen.push(p); | ||
| if (p.endsWith('/fixtures/esm')) return fakeModule; | ||
| }); | ||
|
|
||
| const result = await importModule(getFilepath('esm')); | ||
| assert.deepEqual(result, fakeModule); | ||
| assert.ok(seen.some((p) => p.endsWith('/fixtures/esm'))); | ||
| }); | ||
|
|
||
| it('honors importDefaultOnly when the bundle hit has a default key', async () => { | ||
| setBundleModuleLoader(() => ({ default: { greet: 'hi' }, other: 'x' })); | ||
|
|
||
| const result = await importModule(getFilepath('esm'), { importDefaultOnly: true }); | ||
| assert.deepEqual(result, { greet: 'hi' }); | ||
| }); | ||
|
|
||
| it('unwraps __esModule double-default shape', async () => { | ||
| setBundleModuleLoader(() => ({ | ||
| default: { __esModule: true, default: { fn: 'bundled' } }, | ||
| })); | ||
|
|
||
| const result = await importModule(getFilepath('esm')); | ||
| assert.equal(result.__esModule, true); | ||
| assert.deepEqual(result.default, { fn: 'bundled' }); | ||
| }); | ||
|
|
||
| it('falls through to normal import when loader returns undefined', async () => { | ||
| setBundleModuleLoader(() => undefined); | ||
|
|
||
| const result = await importModule(getFilepath('esm')); | ||
| assert.ok(result); | ||
| }); | ||
|
|
||
| it('short-circuits importResolve so bundled paths need not exist on disk', async () => { | ||
| const fakeModule = { virtual: true }; | ||
| setBundleModuleLoader((p) => (p === 'virtual/not-on-disk' ? fakeModule : undefined)); | ||
|
|
||
| const result = await importModule('virtual/not-on-disk'); | ||
| assert.deepEqual(result, fakeModule); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
isESMvariable is set tofalsewhen a loader is provided, but it is not restored to its original state whensetBundleModuleLoader(undefined)is called. This can lead to persistent side effects in the current process, which is particularly problematic for test isolation (as seen in theafterEachcleanup in the new tests). Consider capturing the initial detected value ofisESMand restoring it when the loader is unset.