diff --git a/packages/compiler/src/node-islands/probe-recorder.ts b/packages/compiler/src/node-islands/probe-recorder.ts index 1299c183f..499de3593 100644 --- a/packages/compiler/src/node-islands/probe-recorder.ts +++ b/packages/compiler/src/node-islands/probe-recorder.ts @@ -80,9 +80,11 @@ function internalCaller() { const fnToString = Function.prototype.toString; const hasOwn = Object.prototype.hasOwnProperty; // A getter that only reads a binding (esbuild's and tsc's export getters): -// reading it again reads the same binding. Any other getter can run code -// on every read, so its member is not snapshotted. -const BINDING_GETTER = /^(?:\(\)\s*=>\s*[\w$.]+|function\s*[\w$]*\s*\(\)\s*\{\s*return\s+[\w$.]+(?:\[[\w$]+\])?\s*;?\s*\}|get\s+[\w$]+\s*\(\)\s*\{\s*return\s+[\w$.]+\s*;?\s*\})$/; +// reading it again reads the same binding. A member read ('a.b', 'a[b]') +// can invoke another accessor that runs code on every read, so only a bare +// identifier qualifies. Any other getter can also run code on every read, +// so its member is not snapshotted. +const BINDING_GETTER = /^(?:\(\)\s*=>\s*[A-Za-z_$][\w$]*|function\s*[\w$]*\s*\(\)\s*\{\s*return\s+[A-Za-z_$][\w$]*\s*;?\s*\}|get\s+[\w$]+\s*\(\)\s*\{\s*return\s+[A-Za-z_$][\w$]*\s*;?\s*\})$/; function bindingGetter(fn) { let source = ""; try { source = fnToString.call(fn); } catch { return false; } @@ -463,20 +465,31 @@ exports.probe = function probe(input) { return [name, m]; }); + // What a member of a module value reads now: a data property's value, or + // a binding getter's (esbuild's and tsc's export getters). Any other + // accessor can run code, so it is not part of a shape. + const MISSING = {}; + const memberValue = (o, k) => { + const d = Object.getOwnPropertyDescriptor(o, k); + if (d === undefined) return MISSING; + if ("value" in d) return d.value; + if (typeof d.get !== "function" || !bindingGetter(d.get)) return MISSING; + try { return d.get.call(o); } catch { return MISSING; } + }; + // A module's exported values right after its load, at the depth the // snapshot covers (the module value's members, and theirs for objects). const exportShape = (v) => { const shape = []; if (v === null || (typeof v !== "object" && typeof v !== "function")) return shape; for (const k of Reflect.ownKeys(v)) { - const d = Object.getOwnPropertyDescriptor(v, k); - if (d === undefined || !("value" in d)) continue; - shape.push([v, k, d.value]); - const m = d.value; + const m = memberValue(v, k); + if (m === MISSING) continue; + shape.push([v, k, m]); if (m !== null && typeof m === "object") for (const k2 of Reflect.ownKeys(m)) { - const d2 = Object.getOwnPropertyDescriptor(m, k2); - if (d2 !== undefined && "value" in d2) shape.push([m, k2, d2.value]); + const m2 = memberValue(m, k2); + if (m2 !== MISSING) shape.push([m, k2, m2]); } } return shape; @@ -537,10 +550,7 @@ exports.probe = function probe(input) { // earlier snapshot no longer holds, and this load's change has to happen // where the program imports it). for (const [earlier, shape] of shapes) { - const changed = shape.some(([o, k, v]) => { - const d = Object.getOwnPropertyDescriptor(o, k); - return d === undefined || !("value" in d) || !Object.is(d.value, v); - }); + const changed = shape.some(([o, k, v]) => !Object.is(memberValue(o, k), v)); if (!changed) continue; delete earlier.root; earlier.reason = "a later island module's load changes its exports"; diff --git a/tests/fixtures/node-island/deferral/getter-member/main.ts b/tests/fixtures/node-island/deferral/getter-member/main.ts new file mode 100644 index 000000000..96223673c --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/main.ts @@ -0,0 +1,3 @@ +import counter from "counter-pkg"; + +console.log(counter.label, counter.count, counter.count); diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts new file mode 100644 index 000000000..82afbe16d --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.d.ts @@ -0,0 +1,2 @@ +declare const counter: { readonly count: number; readonly label: string }; +export default counter; diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js new file mode 100644 index 000000000..8ad752913 --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/index.js @@ -0,0 +1,7 @@ +"use strict"; +let reads = 0; +// The export getter reads a member, not a binding: that member's getter +// counts the read, so it is not a plain export getter. +const state = { get count() { return ++reads; } }; +exports.label = "counter"; +Object.defineProperty(exports, "count", { enumerable: true, get: () => state.count }); diff --git a/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json new file mode 100644 index 000000000..50ce07f31 --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/node_modules/counter-pkg/package.json @@ -0,0 +1 @@ +{ "name": "counter-pkg", "version": "1.0.0", "main": "index.js", "types": "index.d.ts" } diff --git a/tests/fixtures/node-island/deferral/getter-member/package.json b/tests/fixtures/node-island/deferral/getter-member/package.json new file mode 100644 index 000000000..6d5d7c46b --- /dev/null +++ b/tests/fixtures/node-island/deferral/getter-member/package.json @@ -0,0 +1 @@ +{ "name": "deferral-getter-member", "private": true, "type": "module", "scriptc": { "packages": { "counter-pkg": "node" } } } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/main.ts b/tests/fixtures/node-island/deferral/mutate-getter/main.ts new file mode 100644 index 000000000..f7dbae8e2 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/main.ts @@ -0,0 +1,4 @@ +import { config } from "pkg-a"; +import { ok } from "pkg-b"; + +console.log(config.mode, ok); diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts new file mode 100644 index 000000000..8b403d629 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.d.ts @@ -0,0 +1 @@ +export declare const config: { mode: string }; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js new file mode 100644 index 000000000..c46d72b3a --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/index.js @@ -0,0 +1,4 @@ +"use strict"; +// pkg-a exports through a binding getter over its own object. +const config = { mode: "initial" }; +Object.defineProperty(exports, "config", { enumerable: true, get: () => config }); diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json new file mode 100644 index 000000000..e0270f674 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-a/package.json @@ -0,0 +1 @@ +{ "name": "pkg-a", "version": "1.0.0", "main": "index.js", "types": "index.d.ts" } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts new file mode 100644 index 000000000..f29ace298 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.d.ts @@ -0,0 +1 @@ +export declare const ok: boolean; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js new file mode 100644 index 000000000..749c51d3c --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/index.js @@ -0,0 +1,4 @@ +"use strict"; +// Its load changes what pkg-a exports, through pkg-a's binding getter. +require("pkg-a").config.mode = "set by pkg-b"; +exports.ok = true; diff --git a/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json new file mode 100644 index 000000000..ea2784527 --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/node_modules/pkg-b/package.json @@ -0,0 +1 @@ +{ "name": "pkg-b", "version": "1.0.0", "main": "index.js", "types": "index.d.ts", "dependencies": { "pkg-a": "1.0.0" } } diff --git a/tests/fixtures/node-island/deferral/mutate-getter/package.json b/tests/fixtures/node-island/deferral/mutate-getter/package.json new file mode 100644 index 000000000..0416d97bc --- /dev/null +++ b/tests/fixtures/node-island/deferral/mutate-getter/package.json @@ -0,0 +1 @@ +{ "name": "deferral-mutate-getter", "private": true, "type": "module", "scriptc": { "packages": { "pkg-a": "node", "pkg-b": { "run": "node", "evaluate": "lazy" } } } } diff --git a/tests/harness/node-island-deferral.test.ts b/tests/harness/node-island-deferral.test.ts index c17b847fc..93dce44cb 100644 --- a/tests/harness/node-island-deferral.test.ts +++ b/tests/harness/node-island-deferral.test.ts @@ -75,11 +75,27 @@ describe.skipIf(skip)("deferred Node island modules", () => { expect(await run(binary)).toBe("set by pkg-b true\n"); }, 300_000); + test("a load that changes an object exported through a getter defers neither", async () => { + const { binary, trace } = await build("mutate-getter"); + expect(trace.some((l) => /eager .*pkg-a.*a later island module's load changes/.test(l))).toBe( + true, + ); + expect( + trace.some((l) => /eager .*pkg-b.*changes another island module's exports/.test(l)), + ).toBe(true); + expect(await run(binary)).toBe("set by pkg-b true\n"); + }, 300_000); + test("an export getter that runs code runs on every read", async () => { const { binary } = await build("getter"); expect(await run(binary)).toBe("counter 1 2\n"); }, 300_000); + test("an export getter that reads a member runs on every read", async () => { + const { binary } = await build("getter-member"); + expect(await run(binary)).toBe("counter 1 2\n"); + }, 300_000); + test("a package file missing when the build probes keeps the module eager", async () => { const { binary, trace } = await build("missing-file"); expect(trace.some((l) => /eager .*probe-pkg.*existsSync/.test(l))).toBe(true);