Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions packages/utils/src/import.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The isESM variable is set to false when a loader is provided, but it is not restored to its original state when setBundleModuleLoader(undefined) is called. This can lead to persistent side effects in the current process, which is particularly problematic for test isolation (as seen in the afterEach cleanup in the new tests). Consider capturing the initial detected value of isESM and restoring it when the loader is unset.

Comment on lines +409 to +412
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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.ts

Repository: eggjs/egg

Length of output: 1118


🏁 Script executed:

# Find all calls to setBundleModuleLoader to understand usage pattern
rg -n 'setBundleModuleLoader' --type ts --type js

Repository: eggjs/egg

Length of output: 876


🏁 Script executed:

# Check what _snapshotModuleLoader is and where it's defined
rg -n '_snapshotModuleLoader' packages/utils/src/import.ts

Repository: 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 -n

Repository: 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.ts

Repository: 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 -n

Repository: 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 -100

Repository: 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 -n

Repository: 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.ts

Repository: 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 -n

Repository: 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.ts

Repository: 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 -n

Repository: 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 -20

Repository: eggjs/egg

Length of output: 309


Restore isESM when unregistering the bundle loader.

Line 411 only sets isESM = false on registration. When setBundleModuleLoader(undefined) is called (as in the afterEach in tests), isESM stays false, causing subsequent importModule() calls to incorrectly route through require() instead of import(), breaking imports of ESM-only modules.

Additionally, replace as any casts on globalThis.

Lines 410 and 415 use (globalThis as any) which violates the TypeScript guideline against any types. Define a proper type or augmentation for the globalThis properties instead.

🛠️ 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
Verify each finding against the current code and only fix it if needed.

In `@packages/utils/src/import.ts` around lines 409 - 412, setBundleModuleLoader
currently sets (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ and flips isESM
= false on registration but never restores isESM when loader is unset, causing
importModule to incorrectly use require(); update setBundleModuleLoader(loader)
to set __EGG_BUNDLE_MODULE_LOADER__ and set isESM = loader ? false : true (or
restore previous ESM state) so unregistering (loader === undefined) resets
isESM; also remove the (globalThis as any) casts by adding a globalThis
augmentation that declares __EGG_BUNDLE_MODULE_LOADER__?: BundleModuleLoader and
use that typed property instead of any to access the global.


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
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 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.ts

Repository: 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 tsx

Repository: 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 -n

Repository: 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 -n

Repository: 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.ts

Repository: 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 -n

Repository: 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 -n

Repository: 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.ts

Repository: 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.ts

Repository: 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 -n

Repository: eggjs/egg

Length of output: 1158


Replace globalThis as any with a typed global declaration.

Lines 410 and 415 use as any casts, which weakens strict typing. The __EGG_BUNDLE_MODULE_LOADER__ global is only used within this file and can be safely typed via declare global.

♻️ 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
Verify each finding against the current code and only fix it if needed.

In `@packages/utils/src/import.ts` around lines 410 - 415, The code uses
(globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ which weakens typing; declare a
typed global interface for __EGG_BUNDLE_MODULE_LOADER__ (e.g. interface Global {
__EGG_BUNDLE_MODULE_LOADER__?: BundleModuleLoader }) via declare global so you
can replace both (globalThis as any).__EGG_BUNDLE_MODULE_LOADER__ occurrences
with properly typed accesses, and keep the existing logic in importModule and
the loader assignment (references: __EGG_BUNDLE_MODULE_LOADER__,
BundleModuleLoader, importModule, loader, isESM).

if (_bundleModuleLoader) {
const hit = _bundleModuleLoader(filepath.replaceAll('\\', '/'));
Comment on lines +415 to +417
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

There is a consistency issue when multiple instances of @eggjs/utils are present in the same process (e.g., under Turbopack). While the bundle loader is shared via globalThis, the isESM = false override in setBundleModuleLoader only affects the current module instance. Other instances will still have isESM = true, leading to inconsistent behavior in importModule and importResolve if the loader falls through. Consider synchronizing this state globally or checking the global loader presence dynamically when evaluating ESM vs CJS logic.

  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) {
Expand Down
1 change: 1 addition & 0 deletions packages/utils/test/__snapshots__/index.test.ts.snap
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ exports[`test/index.test.ts > export all > should keep checking 1`] = `
"importResolve",
"isESM",
"isSupportTypeScript",
"setBundleModuleLoader",
"setSnapshotModuleLoader",
]
`;
63 changes: 63 additions & 0 deletions packages/utils/test/bundle-import.test.ts
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);
});
});
Loading