Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
2 changes: 1 addition & 1 deletion scripts/build/deps/webkit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
* for local mode. Override via `--webkit-version=<hash>` to test a branch.
* From https://github.com/oven-sh/WebKit releases.
*/
export const WEBKIT_VERSION = "f20ce7744553c910bcf16a33faf976af208de091";
export const WEBKIT_VERSION = "fb1167ebf2cb9edc1f6771a2c11771b024693ae0";

/**
* WebKit (JavaScriptCore) — the JS engine.
Expand Down
2 changes: 1 addition & 1 deletion src/js/bun/sql.ts
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ const SQL = function SQL(
if ((values?.length ?? 0) === 0) {
flags |= SQLQueryFlags.simple;
}
const query = new Query(
const query = new Query<import("internal/sql/shared.ts").SQLResultArray<any>, any>(
strings,
values,
flags,
Expand Down
54 changes: 53 additions & 1 deletion test/cli/hot/hot.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import { spawn } from "bun";
import { beforeEach, expect, it } from "bun:test";
import { copyFileSync, cpSync, readFileSync, renameSync, rmSync, unlinkSync, writeFileSync } from "fs";
import { bunEnv, bunExe, isDebug, isWindows, tmpdirSync, waitForFileToExist } from "harness";
import { bunEnv, bunExe, isDebug, isWindows, tempDir, tmpdirSync, waitForFileToExist } from "harness";
import { join } from "path";

const timeout = isDebug ? Infinity : 10_000;
Expand Down Expand Up @@ -776,3 +776,55 @@ ${Buffer.alloc(counter * 2, " ").toString()}throw new Error(${counter});`,
},
longTimeout,
);

it(
"should import a module again after a hot reload while its import() was still loading its dependencies",
async () => {
using dir = tempDir("hot-reload-import-in-flight", {
"a.mjs": `import "./dependency.mjs"; export const evaluation = (globalThis.evaluations = (globalThis.evaluations ?? 0) + 1);`,
"dependency.mjs": `export {};`,
"entry.mjs": `
import { readFileSync, writeFileSync } from "node:fs";
globalThis.runs = (globalThis.runs ?? 0) + 1;
if (globalThis.runs === 1) {
Bun.plugin({
name: "hold the dependency's load open until the reload",
setup(build) {
build.onLoad({ filter: /dependency\\.mjs$/ }, () => {
const loaded = { contents: "export {}", loader: "js" };
if (globalThis.dependencyMayLoad) return loaded;
const { promise, resolve } = Promise.withResolvers();
globalThis.dependencyMayLoad = () => resolve(loaded);
writeFileSync(import.meta.path, readFileSync(import.meta.path));
return promise;
});
},
});
globalThis.inFlight = import("./a.mjs");
} else {
globalThis.dependencyMayLoad();
try {
console.log("in flight: evaluation", (await globalThis.inFlight).evaluation);
console.log("next: evaluation", (await import("./a.mjs")).evaluation);
process.exit(0);
} catch (error) {
// --hot would keep the process alive after an uncaught error.
console.log("rejected:", error);
process.exit(1);
}
}
`,
});
await using proc = spawn({
cmd: [bunExe(), "--hot", "entry.mjs"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "inherit",
});
const [stdout, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]);
expect(stdout).toBe("in flight: evaluation 1\nnext: evaluation 2\n");
expect(exitCode).toBe(0);
},
timeout,
);
91 changes: 91 additions & 0 deletions test/js/bun/plugin/plugins.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1064,3 +1064,94 @@ it("object loader: an error thrown by a getter on the exports object rejects the
});
expect(() => require("object-loader-throwing-esmodule")).toThrow(boom);
});

it.concurrent("build.module() of a module whose import() is still loading its dependencies", async () => {
using dir = tempDir("plugin-module-import-in-flight", {
"a.ts": `import "./dependency"; export const from = "file";`,
"dependency.ts": `export {};`,
"entry.ts": `
import { join } from "node:path";
const dependencyRequested = Promise.withResolvers<void>();
const dependencyMayLoad = Promise.withResolvers<void>();
Bun.plugin({
name: "hold the dependency's load open",
setup(build) {
build.onLoad({ filter: /dependency\\.ts$/ }, async () => {
dependencyRequested.resolve();
await dependencyMayLoad.promise;
return { contents: "export {}", loader: "ts" };
});
},
});

const a = join(import.meta.dir, "a.ts");
const inFlight = import(a);
await dependencyRequested.promise;
Bun.plugin({
name: "replace a.ts",
setup(build) {
build.module(a, () => ({ exports: { from: "build.module()" }, loader: "object" }));
},
});
dependencyMayLoad.resolve();

console.log("in flight:", (await inFlight).from);
console.log("next:", (await import(a)).from);
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "entry.ts"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({
stdout: "in flight: file\nnext: build.module()\n",
stderr: "",
exitCode: 0,
});
});

// The loader resolves a path that import() has resolved twice more, so onResolve is fed its own results: a → b → c → d.
// That leaves d.mjs registered under a key other than the one it was asked for by, which is what this is about.
it.concurrent(
"import() after delete require.cache of a module that onResolve redirected a resolved path to",
async () => {
using dir = tempDir("plugin-onresolve-chain-removed", {
"a.mjs": `export const from = "a.mjs";`,
"b.mjs": `export const from = "b.mjs";`,
"c.mjs": `export const from = "c.mjs";`,
"d.mjs": `export const from = "d.mjs, evaluation " + (globalThis.evaluations = (globalThis.evaluations ?? 0) + 1);`,
"entry.ts": `
import { basename, join } from "node:path";
const next = { "a.mjs": "b.mjs", "b.mjs": "c.mjs", "c.mjs": "d.mjs" };
Bun.plugin({
name: "redirect a path that is already resolved, again and again",
setup(build) {
build.onResolve({ filter: /[abc]\\.mjs$/ }, ({ path }) => ({ path: join(import.meta.dir, next[basename(path)]) }));
Comment thread
dylan-conway marked this conversation as resolved.
},
});

const a = join(import.meta.dir, "a.mjs");
console.log("first:", (await import(a)).from);
console.log("deleted:", delete require.cache[join(import.meta.dir, "d.mjs")]);
console.log("again:", (await import(a)).from);
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "entry.ts"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect({ stdout, stderr, exitCode }).toEqual({
stdout: "first: d.mjs, evaluation 1\ndeleted: true\nagain: d.mjs, evaluation 2\n",
stderr: "",
exitCode: 0,
});
},
);
43 changes: 43 additions & 0 deletions test/js/bun/test/mock/mock-module.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -497,3 +497,46 @@ test.concurrent(
expect(exitCode).toBe(0);
},
);

test.concurrent("mock.module() of a module whose import() is still loading its dependencies", async () => {
using dir = tempDir("mock-module-import-in-flight", {
"a.ts": `import "./dependency"; export const a = "real-a";`,
"dependency.ts": `export {};`,
"in-flight.test.ts": `
import { expect, mock, test } from "bun:test";

test("the import in flight gets the module it was loading, the next one gets the mock", async () => {
const dependencyRequested = Promise.withResolvers<void>();
const dependencyMayLoad = Promise.withResolvers<void>();
Bun.plugin({
name: "hold the dependency's load open",
setup(build) {
build.onLoad({ filter: /dependency\\.ts$/ }, async () => {
dependencyRequested.resolve();
await dependencyMayLoad.promise;
return { contents: "export {}", loader: "ts" };
});
},
});

const inFlight = import("./a");
await dependencyRequested.promise;
mock.module("./a", () => ({ a: "mocked-a" }));

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.

🟣 pre-existing, not blocking: pre-existing: a test that calls mock.module() while the target's own fetch (its onLoad) is still pending keeps getting the real module from the next import(), silently, on base and after this PR. mock.module at BunPlugin.cpp:522 finds no registry entry because the C++ loader registers a key only after its fetch settles (ModuleLoader.cpp:494-507), so nothing is removed; the pending fetch then registers the real module and import() at ZigGlobalObject.cpp:3686 reuses it. Same for build.module() (BunPlugin.cpp:162) and a --hot reload during the fetch, which re-registers the pre-reload module. Fix: cover this sibling of the in-flight case, e.g. register the entry before fetch (the FIXME) or note it as excluded in the PR.
A small fix can ride a push you are already making; otherwise a short reply is enough.

Why this was flagged

Trigger: a plugin onLoad matching a.ts itself returns a pending promise; test code runs const p = import("./a"); mock.module("./a", factory); before that promise resolves. JSMock__jsModuleMock calls findLoadedESModule (BunPlugin.cpp:513-543); registryEntry(specifierIdent) at BunPlugin.cpp:522 is null because, per the comment at src/jsc/bindings/ModuleLoader.cpp:494-499, the loader creates the registry entry only inside ModuleLoadTopSettled after the embedder fetch promise resolves. staleESMEntry stays false, so removeEntry at BunPlugin.cpp:745 never runs; only addModuleMock at :753 happens. When the onLoad promise later resolves, Bun__onFulfillAsyncModule (ModuleLoader.cpp:472-531) resolves the fetch with the real source and provideFetch registers the real a.ts. The next import("./a") resolves through resolveVirtualModule at ZigGlobalObject.cpp:3683 to the same key and requestImportModule at :3686 finds the registered real record, so the user gets "real-a" where they asked for the mock; no error is raised. The base branch behaves the same, so this is pre-existing.

Verification: pre-existing: triggers when mock.module() (or build.module()) targets a module whose own fetch is still pending; the base fails the same way. BunPlugin.cpp:522-524 finds no registry entry, so staleESMEntry stays false and removeEntry never runs. ModuleLoader.cpp:494-507: the loader does not create a registry entry until after the fetch promise resolves. Consequence: wrong behavior, silent.

dependencyMayLoad.resolve();

expect((await inFlight).a).toBe("real-a");
expect((await import("./a")).a).toBe("mocked-a");
});
`,
});
await using proc = Bun.spawn({
cmd: [bunExe(), "test", "./in-flight.test.ts"],
cwd: String(dir),
env: bunEnv,
stdout: "pipe",
stderr: "pipe",
});
const [, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(stderr).toContain(" 1 pass");
expect(exitCode).toBe(0);
});
Loading