mirror of
https://github.com/openclaw/openclaw.git
synced 2026-05-06 06:40:44 +00:00
fixup! perf(plugins): native-require fast path respects tryNative=false
Review feedback from @chatgpt-codex-connector (P1): callers that pass `tryNative: false` rely on jiti's alias rewriting (e.g. `bundled-capability-runtime` in Vitest+dist mode narrows the SDK slice through shim aliases). Route everything through the jiti loader when `tryNative` is false so those rewrites still apply. Review feedback from @greptile-apps (P2): forward the full argument tuple through to the jiti fallback with `...rest` so any future loader option argument is not silently dropped by the wrapper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -258,4 +258,64 @@ describe("getCachedPluginJitiLoader", () => {
|
||||
expect(result.fromJiti).toBe(true);
|
||||
expect(jitiLoader).toHaveBeenCalledWith("/repo/dist/extensions/demo/api.js");
|
||||
});
|
||||
|
||||
it("skips the native-require fast path when tryNative is explicitly false", async () => {
|
||||
const jitiLoader = vi.fn(() => ({ fromJiti: true }));
|
||||
const createJiti = vi.fn(() => jitiLoader);
|
||||
vi.doMock("jiti", () => ({ createJiti }));
|
||||
const nativeStub = vi.fn(() => ({ ok: true, moduleExport: { fromNative: true } }));
|
||||
vi.doMock("./native-module-require.js", () => ({
|
||||
isJavaScriptModulePath: () => true,
|
||||
tryNativeRequireJavaScriptModule: nativeStub,
|
||||
}));
|
||||
const { getCachedPluginJitiLoader } = await importFreshModule<
|
||||
typeof import("./jiti-loader-cache.js")
|
||||
>(import.meta.url, "./jiti-loader-cache.js?scope=native-require-opt-out");
|
||||
|
||||
const cache = new Map();
|
||||
const loader = getCachedPluginJitiLoader({
|
||||
cache,
|
||||
modulePath: "/repo/dist/extensions/demo/api.js",
|
||||
importerUrl: "file:///repo/src/plugins/bundled-capability-runtime.ts",
|
||||
jitiFilename: "file:///repo/src/plugins/bundled-capability-runtime.ts",
|
||||
aliasMap: { "openclaw/plugin-sdk": "/repo/shim.js" },
|
||||
tryNative: false,
|
||||
});
|
||||
|
||||
const result = loader("/repo/dist/extensions/demo/api.js") as { fromJiti: boolean };
|
||||
expect(result.fromJiti).toBe(true);
|
||||
// With tryNative: false the wrapper must route every target through jiti
|
||||
// so its alias rewrites still apply; native require must not be consulted.
|
||||
expect(nativeStub).not.toHaveBeenCalled();
|
||||
expect(jitiLoader).toHaveBeenCalledWith("/repo/dist/extensions/demo/api.js");
|
||||
});
|
||||
|
||||
it("forwards extra loader arguments through to the jiti fallback", async () => {
|
||||
const jitiLoader = vi.fn(() => ({ fromJiti: true }));
|
||||
const createJiti = vi.fn(() => jitiLoader);
|
||||
vi.doMock("jiti", () => ({ createJiti }));
|
||||
vi.doMock("./native-module-require.js", () => ({
|
||||
isJavaScriptModulePath: () => true,
|
||||
tryNativeRequireJavaScriptModule: () => ({ ok: false }),
|
||||
}));
|
||||
const { getCachedPluginJitiLoader } = await importFreshModule<
|
||||
typeof import("./jiti-loader-cache.js")
|
||||
>(import.meta.url, "./jiti-loader-cache.js?scope=native-require-rest-args");
|
||||
|
||||
const cache = new Map();
|
||||
const loader = getCachedPluginJitiLoader({
|
||||
cache,
|
||||
modulePath: "/repo/dist/extensions/demo/api.js",
|
||||
importerUrl: "file:///repo/src/plugins/public-surface-loader.ts",
|
||||
jitiFilename: "file:///repo/src/plugins/public-surface-loader.ts",
|
||||
});
|
||||
|
||||
const loose = loader as unknown as (t: string, ...a: unknown[]) => unknown;
|
||||
loose("/repo/dist/extensions/demo/api.js", { hint: "x" }, 42);
|
||||
expect(jitiLoader).toHaveBeenCalledWith(
|
||||
"/repo/dist/extensions/demo/api.js",
|
||||
{ hint: "x" },
|
||||
42,
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -79,19 +79,27 @@ export function getCachedPluginJitiLoader(params: {
|
||||
...buildPluginLoaderJitiOptions(aliasMap),
|
||||
tryNative,
|
||||
});
|
||||
// The returned loader prefers native require() for already-compiled JS
|
||||
// artifacts (the bundled plugin public surfaces shipped in dist/) because
|
||||
// jiti's transform pipeline provides no value for output that is already
|
||||
// plain JS and adds several seconds of per-load overhead on slower hosts.
|
||||
// Jiti stays on the hot path for TS / TSX and for the small set of
|
||||
// require(esm)/async-module fallbacks `tryNativeRequireJavaScriptModule`
|
||||
// declines to handle.
|
||||
const loader = ((target: string) => {
|
||||
// When the caller has explicitly opted out of native loading (for example
|
||||
// `bundled-capability-runtime` in Vitest+dist mode, which depends on
|
||||
// jiti's alias rewriting to surface a narrow SDK slice), route every
|
||||
// target through jiti so those alias rewrites still apply.
|
||||
if (!tryNative) {
|
||||
params.cache.set(scopedCacheKey, jitiLoader);
|
||||
return jitiLoader;
|
||||
}
|
||||
// Otherwise prefer native require() for already-compiled JS artifacts
|
||||
// (the bundled plugin public surfaces shipped in dist/). jiti's transform
|
||||
// pipeline provides no value for output that is already plain JS and adds
|
||||
// several seconds of per-load overhead on slower hosts. jiti still runs
|
||||
// for TS / TSX sources and for the small set of require(esm) /
|
||||
// async-module fallbacks `tryNativeRequireJavaScriptModule` declines to
|
||||
// handle.
|
||||
const loader = ((target: string, ...rest: unknown[]) => {
|
||||
const native = tryNativeRequireJavaScriptModule(target);
|
||||
if (native.ok) {
|
||||
return native.moduleExport;
|
||||
}
|
||||
return jitiLoader(target);
|
||||
return (jitiLoader as (t: string, ...a: unknown[]) => unknown)(target, ...rest);
|
||||
}) as PluginJitiLoader;
|
||||
params.cache.set(scopedCacheKey, loader);
|
||||
return loader;
|
||||
|
||||
Reference in New Issue
Block a user