diff --git a/.changeset/7918-zod-lazy-getter-identity.md b/.changeset/7918-zod-lazy-getter-identity.md new file mode 100644 index 0000000000..aaba556a64 --- /dev/null +++ b/.changeset/7918-zod-lazy-getter-identity.md @@ -0,0 +1,54 @@ +--- +'@object-ui/types': patch +--- + +Two of the ten `z.lazy` exports in the zod node face now memoise their getter, so their +public `.unwrap()` compares by identity (objectui#7918). The other eight are **deliberately +unchanged** — measured, memoising them is a module-load `ReferenceError`. + +The card that found this did not claim the ten were wrong. It asked whether the spelling was +buying a temporal-dead-zone dodge, and that check is what shipped. Each of the ten was +rewritten in place to `const inner = ; z.lazy(() => inner)`, the package rebuilt, and +the built barrel imported in a fresh process. **Eight refuse to load.** Seven name the very +const being declared (`children: z.array(TreeNodeSchema)` sits inside `TreeNodeSchema`'s own +initialiser); `SchemaNodeSchema` names `BaseSchemaCore`, which `base.zod.ts` declares below +it. For those eight the `z.lazy` is load-bearing, so `ActionSchema`, `AppMenuItemSchema`, +`FilterGroupSchema`, `MenuItemSchema`, `NavLinkSchema`, `NavigationMenuItemSchema`, +`SchemaNodeSchema` and `TreeNodeSchema` keep the spelling they have. The two that loaded +clean are memoised: `FilterBuilderConditionSchema` is not recursive at all, and +`NavigationItemSchema` already defers its self-reference through an inner +`z.lazy(() => NavigationItemSchema)` on `children`. + +⚠️ Two corrections to the finding, both measured, both worth more than the edit: + +**The recursion point was already identity-comparable, through the right handle.** +`zod@4.4.3` caches a lazy's resolved inner type on `def._cachedInner` — its own comment says +this preserves "identity for cycle detection on recursive schemas" — and `S._zod.innerType` +reads that cache. It is stable for all ten, including the eight, and survives `.describe()` +clones. What is *not* stable is `S.unwrap()`, because `ZodLazy` defines it as +`() => inst._zod.def.getter()`, going around the cache (`ZodPromise` spells its own as a +stored field). So a schema walker can recognise the recursion point today by reading +`_zod.innerType`; the objectui#7581 false negative — `ActionSchema` reported "not exported by +name" when it plainly is — was the wrong handle, not an unrecognisable schema. Memoising is +still worth doing where it is free, because it makes the public `.unwrap()` honest. + +**The "rebuilt on every parse" cost does not exist.** The finding recorded, explicitly +unmeasured, that a document with N nodes reconstructs the recursive sub-schema N times. It +does not: the getter runs **once per lazy for the life of the process**, via the same +`_cachedInner` — measured at one call during the first parse of a 13-node document and zero +during the second. Wall clock agrees. `NavigationItemSchema` over a 73-node document, +memoised versus not, medians of nine trials of 200 parses: 149,973 ns versus 140,293 ns per +parse — ratio 0.94x, with the ranges overlapping. Those are shared-box seconds, so the +absolutes are not idle-machine figures; the ratio is the reading, and the reading is "no +difference". There is no parse-time win here, and anyone pricing the strict face +(objectui#7935 / objectstack#5250) should strike this from the input list. + +No accept/reject behaviour moves — a memoised getter changes schema *identity*, not what is +declared or admitted. The measurement, the eight `ReferenceError` messages, the identity +matrix and an executable reproduction of both the TDZ mechanism and the once-per-process +getter are pinned in `packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts`. + +⚠️ Also settled while locating the ten: `AppMenuItemSchema` has no declaration of its own — +it is the barrel alias of `app.zod.ts`'s `MenuItemSchema`, while the barrel's own +`MenuItemSchema` is `overlay.zod.ts`'s. Two different schemas, so the list really is ten +entries and not nine. diff --git a/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts new file mode 100644 index 0000000000..e2a9e3b396 --- /dev/null +++ b/packages/types/src/__tests__/zod-lazy-getter-identity-7918.test.ts @@ -0,0 +1,279 @@ +/** + * objectui#7918 — the `z.lazy` getter-identity ledger for the zod node face. + * + * Ten exports of `@object-ui/types/zod` are `z.lazy(() => …)` whose getter builds + * a NEW schema on every call, so `S._zod.def.getter() !== S._zod.def.getter()`. + * The card did NOT claim that was wrong — it asked whether the spelling was + * dodging a temporal-dead-zone error, and made that check the deliverable. This + * file is the answer, kept executable so it cannot rot. Three things were + * settled, and two of them contradict the card. + * + * ## 1. The TDZ question: yes for eight of the ten + * + * Each of the ten was rewritten in place to the obvious memoisation + * — `const inner = ; z.lazy(() => inner)` — the package rebuilt, and the + * built barrel imported in a fresh process. EIGHT refuse to load: + * + * ActionSchema ReferenceError: Cannot access 'ActionSchema' before initialization + * AppMenuItemSchema ReferenceError: Cannot access 'MenuItemSchema' before initialization + * FilterGroupSchema ReferenceError: Cannot access 'FilterGroupSchema' before initialization + * MenuItemSchema ReferenceError: Cannot access 'MenuItemSchema' before initialization + * NavLinkSchema ReferenceError: Cannot access 'NavLinkSchema' before initialization + * NavigationMenuItemSchema ReferenceError: Cannot access 'NavigationMenuItemSchema' before initialization + * SchemaNodeSchema ReferenceError: Cannot access 'BaseSchemaCore' before initialization + * TreeNodeSchema ReferenceError: Cannot access 'TreeNodeSchema' before initialization + * + * Seven name the very const being declared (`children: z.array(TreeNodeSchema)` + * sits inside `TreeNodeSchema`'s own initialiser); `SchemaNodeSchema` names + * `BaseSchemaCore`, which `base.zod.ts` declares BELOW it. For those eight the + * `z.lazy` is LOAD-BEARING — it is buying a TDZ dodge, not a style — and they + * keep the spelling they have. `mechanism` below reproduces the failure. + * + * The two that loaded clean were memoised: `FilterBuilderConditionSchema` is not + * recursive at all, and `NavigationItemSchema` already defers its self-reference + * through the inner `z.lazy(() => NavigationItemSchema)` on `children`. + * + * ## 2. The card's consequence ① is real, but only for the PUBLIC handle + * + * `zod@4.4.3` caches a lazy's resolved inner type on the shared def + * (`$ZodLazy` -> `util.defineLazy(inst._zod, 'innerType', …)`, which fills + * `def._cachedInner` once). Its own comment says the cache exists to preserve + * "identity for cycle detection on recursive schemas". So: + * + * S._zod.innerType STABLE for all ten, even the eight (internal) + * S.unwrap() fresh object per call for the eight (PUBLIC) + * S._zod.def.getter() fresh object per call for the eight (internal) + * + * `.unwrap()` is unstable because `ZodLazy` defines it as + * `() => inst._zod.def.getter()` — it goes around the cache. (`ZodPromise` + * spells its own as `() => inst._zod.def.innerType`, i.e. a stored field.) + * + * ⇒ A walker that wants to recognise the recursion point by reference CAN do it + * today, for all ten, by reading `_zod.innerType`. The objectui#7581 script's + * false negative — `ActionSchema` reported "not exported by name" when it plainly + * is — was the wrong handle, not an unrecognisable schema. Memoising is still + * worth doing where it is free, because it makes the PUBLIC `.unwrap()` honest. + * + * ## 3. The card's consequence ② does not reproduce + * + * The card recorded, explicitly unmeasured, that "a document with N nodes + * reconstructs the whole recursive sub-schema N times". Measured, it does not: + * the getter runs ONCE per lazy for the lifetime of the process, because of the + * same `_cachedInner`. `consequence2` below pins it — one call during the first + * parse of a 13-node document, zero during the second. + * + * Wall clock agrees. `NavigationItemSchema` parsing a 73-node document, memoised + * vs. not, same tree, same process count, medians of nine trials of 200 parses: + * 149,973 ns vs 140,293 ns per parse — ratio 0.94x, with the two ranges + * overlapping (143,913–153,846 vs 137,266–149,399). Shared-box seconds, so the + * absolutes are not idle-machine figures; the ratio is the reading, and the + * reading is "no difference". ⛔ There is no parse-time win here to claim. + * + * ## If you are here to memoise the other eight + * + * The naive shape cannot work — that is measured above. A shape that COULD is to + * hoist each body to a module const and push the self-reference behind an inner + * `z.lazy(() => X)`, the way `NavigationItemSchema.children` already does. That + * is deliberately NOT taken here: it trades one identity for another, since + * `children` is currently `z.array(TreeNodeSchema)` — whose element IS the + * exported schema — and the rewrite replaces that element with a fresh anonymous + * wrapper. With consequence ② disproved and `_zod.innerType` already stable, + * the remaining prize is only public `.unwrap()` identity. Whoever prices the + * strict face (objectui#7935 / objectstack#5250) should make that trade + * deliberately. Update this ledger in the same change. + */ +import { describe, it, expect } from 'vitest'; +import { z } from 'zod'; +import { + ActionSchema, + AppMenuItemSchema, + FilterBuilderConditionSchema, + FilterGroupSchema, + MenuItemSchema, + NavLinkSchema, + NavigationItemSchema, + NavigationMenuItemSchema, + SchemaNodeSchema, + TreeNodeSchema, +} from '../zod/index.zod.js'; +import { MenuItemSchema as AppMenuItemSource } from '../zod/app.zod.js'; +import { MenuItemSchema as OverlayMenuItemSource } from '../zod/overlay.zod.js'; + +type LazyInternals = { _zod: { def: { getter: () => unknown }; innerType: unknown } }; +type Unwrappable = { unwrap: () => unknown }; + +/** `S._zod.def.getter() === S._zod.def.getter()` — the card's one-line probe. */ +const getterStable = (S: unknown): boolean => { + const { getter } = (S as LazyInternals)._zod.def; + return getter() === getter(); +}; +/** The PUBLIC accessor. `ZodLazy` spells it `() => inst._zod.def.getter()`. */ +const unwrapStable = (S: unknown): boolean => (S as Unwrappable).unwrap() === (S as Unwrappable).unwrap(); +/** zod's own cached handle, filled once into `def._cachedInner`. */ +const innerTypeStable = (S: unknown): boolean => (S as LazyInternals)._zod.innerType === (S as LazyInternals)._zod.innerType; + +const MEMOISED: ReadonlyArray = [ + ['FilterBuilderConditionSchema', FilterBuilderConditionSchema], + ['NavigationItemSchema', NavigationItemSchema], +]; +/** ⛔ Do not "fix" these — each one's `z.lazy` dodges a real ReferenceError. */ +const TDZ_BOUND: ReadonlyArray = [ + ['ActionSchema', ActionSchema], + ['AppMenuItemSchema', AppMenuItemSchema], + ['FilterGroupSchema', FilterGroupSchema], + ['MenuItemSchema', MenuItemSchema], + ['NavLinkSchema', NavLinkSchema], + ['NavigationMenuItemSchema', NavigationMenuItemSchema], + ['SchemaNodeSchema', SchemaNodeSchema], + ['TreeNodeSchema', TreeNodeSchema], +]; + +describe('objectui#7918 · z.lazy getter identity', () => { + // ── Controls ─────────────────────────────────────────────────────────── + // A `false` below is only a reading if a positive control that MUST hit does + // hit. Without this pair, "everything answers false" is equally consistent + // with the probe being broken. + describe('controls', () => { + it('POSITIVE: a z.lazy whose getter returns a module-level constant is stable', () => { + const moduleLevelConst = z.object({ a: z.string() }); + expect(getterStable(z.lazy(() => moduleLevelConst))).toBe(true); + }); + + it('NEGATIVE: a z.lazy whose getter builds fresh is not stable', () => { + expect(getterStable(z.lazy(() => z.object({ a: z.string() })))).toBe(false); + }); + }); + + // ── The mechanism the eight are dodging ──────────────────────────────── + describe('mechanism', () => { + it('the naive memoisation throws when the body names the const being declared', () => { + const naiveMemoisation = (): z.ZodType => { + // `const inner = ; z.lazy(() => inner)` with a self-referencing + // body — the exact shape all ten would take. The body is built EAGERLY, + // so it reads `Recursive` while `Recursive` is still in its temporal + // dead zone. + const inner: z.ZodType = z.object({ + // @ts-expect-error TS2448 — used before declaration, which is the point. + children: z.array(Recursive).optional(), + }); + const Recursive: z.ZodType = z.lazy(() => inner); + return Recursive; + }; + expect(naiveMemoisation).toThrow(ReferenceError); + }); + }); + + // ── The ledger ───────────────────────────────────────────────────────── + describe('ledger', () => { + it.each(MEMOISED)('%s is memoised — public unwrap() compares by identity', (_name, S) => { + expect(getterStable(S)).toBe(true); + expect(unwrapStable(S)).toBe(true); + }); + + it.each(TDZ_BOUND)('%s stays lazy-per-call — memoising it is a module-load ReferenceError', (_name, S) => { + expect(getterStable(S)).toBe(false); + expect(unwrapStable(S)).toBe(false); + }); + }); + + // ── Consequence ①, corrected ─────────────────────────────────────────── + describe('the recursion point IS identity-comparable today, via _zod.innerType', () => { + it.each([...MEMOISED, ...TDZ_BOUND])( + '%s has a stable _zod.innerType even when its getter is not stable', + (_name, S) => { expect(innerTypeStable(S)).toBe(true); }, + ); + + it('the cached identity survives a .describe() clone, which is what zod caches it for', () => { + const clone = TreeNodeSchema.describe('a clone'); + expect((clone as unknown as LazyInternals)._zod.innerType) + .toBe((TreeNodeSchema as unknown as LazyInternals)._zod.innerType); + }); + }); + + // ── Consequence ②, disproved ─────────────────────────────────────────── + describe('the schema is NOT rebuilt per parse', () => { + it('the getter runs once per lazy for the life of the process, not once per node', () => { + let calls = 0; + const Recursive: z.ZodType = z.lazy(() => { + calls += 1; + return z.object({ id: z.string(), children: z.array(Recursive).optional() }); + }); + // 13 nodes: 1 root + 3 children + 9 grandchildren. + const doc = { + id: 'r', + children: Array.from({ length: 3 }, (_, i) => ({ + id: `c${i}`, + children: Array.from({ length: 3 }, (_, j) => ({ id: `g${i}${j}` })), + })), + }; + expect(calls).toBe(0); + expect(Recursive.safeParse(doc).success).toBe(true); + expect(calls).toBe(1); // not 13, and not 4 + expect(Recursive.safeParse(doc).success).toBe(true); + expect(calls).toBe(1); // still 1 — `def._cachedInner` holds it + }); + }); + + // ── What `AppMenuItemSchema` is ──────────────────────────────────────── + // The card lists both `AppMenuItemSchema` and `MenuItemSchema`. Triage could + // not find a declaration named `AppMenuItemSchema` and refused to conclude it + // does not exist. It exists: it is the BARREL ALIAS of `app.zod.ts`'s + // `MenuItemSchema` (`index.zod.ts`: `MenuItemSchema as AppMenuItemSchema`), + // while the barrel's own `MenuItemSchema` comes from `overlay.zod.ts`. Two + // different schemas, so the card's list really is ten entries, not nine. + describe('AppMenuItemSchema is a barrel alias, not a missing declaration', () => { + it('resolves to app.zod.ts MenuItemSchema', () => { + expect(AppMenuItemSchema).toBe(AppMenuItemSource); + }); + + it('is a different schema from the barrel MenuItemSchema, which is overlay.zod.ts', () => { + expect(MenuItemSchema).toBe(OverlayMenuItemSource); + expect(AppMenuItemSchema).not.toBe(MenuItemSchema); + }); + }); + + // ── The two changed schemas still parse what they parsed ─────────────── + describe('memoisation moved no accept/reject behaviour', () => { + it('NavigationItemSchema still accepts a nested navigation tree', () => { + const doc = { + id: 'root', type: 'group', label: 'Root', + children: [ + { id: 'crm', type: 'group', label: 'CRM', + children: [{ id: 'acct', type: 'object', label: 'Accounts', objectName: 'account' }] }, + { type: 'separator' }, + ], + }; + expect(NavigationItemSchema.safeParse(doc).success).toBe(true); + }); + + it('NavigationItemSchema still runs its superRefine on nested children', () => { + // `label` missing on a non-separator child — the refinement, not the + // field declarations, is what refuses this. + const bad = { + id: 'root', type: 'group', label: 'Root', + children: [{ id: 'acct', type: 'object' }], + }; + expect(NavigationItemSchema.safeParse(bad).success).toBe(false); + }); + + it('FilterBuilderConditionSchema still accepts a condition and refuses a bad operator', () => { + expect(FilterBuilderConditionSchema.safeParse( + { field: 'amount', operator: 'greater_than', value: 100 }, + ).success).toBe(true); + expect(FilterBuilderConditionSchema.safeParse( + { field: 'amount', operator: 'not_a_real_operator' }, + ).success).toBe(false); + }); + + it('FilterGroupSchema still nests conditions and sub-groups through the memoised arm', () => { + const group = { + id: 'g1', logic: 'and', + conditions: [ + { field: 'amount', operator: 'greater_than', value: 100 }, + { id: 'g2', logic: 'or', conditions: [{ field: 'stage', operator: 'equals', value: 'won' }] }, + ], + }; + expect(FilterGroupSchema.safeParse(group).success).toBe(true); + }); + }); +}); diff --git a/packages/types/src/zod/app.zod.ts b/packages/types/src/zod/app.zod.ts index c38c63bb4a..f5fe10b7a8 100644 --- a/packages/types/src/zod/app.zod.ts +++ b/packages/types/src/zod/app.zod.ts @@ -38,8 +38,25 @@ export const NavigationItemTypeSchema = z.enum([ /** * Navigation Item Schema — unified model aligned with @objectstack/spec. + * + * MEMOISED (objectui#7918): the getter returns a module-level constant, so the + * PUBLIC `NavigationItemSchema.unwrap()` is reference-stable. (`ZodLazy` spells + * `unwrap` as `() => _zod.def.getter()`, going around the cache zod keeps on + * `def._cachedInner`, so an un-memoised lazy hands out a fresh schema per call.) + * Safe here for a reason specific to THIS schema — its self-reference is already + * deferred by the inner `z.lazy(() => NavigationItemSchema)` on `children` below, + * so the body itself names nothing that is still uninitialised when it is built. + * + * ⚠️ Seven of the other nine `z.lazy` exports of this face CANNOT take this + * shape — `MenuItemSchema` further down this very file among them. Those bodies + * name the const being declared DIRECTLY (`children: z.array(MenuItemSchema)`, + * no inner `z.lazy`), so building the body eagerly throws `ReferenceError: + * Cannot access '' before initialization` at module load. Their `z.lazy` + * is buying a TDZ dodge, not a style. Measured one schema at a time in + * `../__tests__/zod-lazy-getter-identity-7918.test.ts` — read that before + * "fixing" any of them to match this one. */ -export const NavigationItemSchema: z.ZodType = z.lazy(() => z.object({ +const NavigationItemObject = z.object({ // Declared optional so a bare `{ type: 'separator' }` — which the spec // accepts, and which carries no identity or text by definition — validates // here too (objectstack#4115). Every OTHER type still requires both; that is @@ -112,7 +129,9 @@ export const NavigationItemSchema: z.ZodType = z.lazy(() => z.object({ }); } } -})); +}); + +export const NavigationItemSchema: z.ZodType = z.lazy(() => NavigationItemObject); /** * Navigation Area Schema — business-domain partition of navigation, DERIVED diff --git a/packages/types/src/zod/complex.zod.ts b/packages/types/src/zod/complex.zod.ts index 6741fcd958..ee9cf17f5d 100644 --- a/packages/types/src/zod/complex.zod.ts +++ b/packages/types/src/zod/complex.zod.ts @@ -264,14 +264,29 @@ export const FilterOperatorSchema = z.enum([ /** * Filter Condition Schema + * + * MEMOISED (objectui#7918): the getter returns a module-level constant, so the + * PUBLIC `FilterBuilderConditionSchema.unwrap()` is reference-stable. (`ZodLazy` + * spells `unwrap` as `() => _zod.def.getter()`, going around the cache zod keeps + * on `def._cachedInner`, so an un-memoised lazy hands out a fresh schema per + * call.) Safe here because this body is NOT recursive — it names only + * `FilterOperatorSchema`, declared above. + * + * ⚠️ `FilterGroupSchema` below CANNOT take this shape, and neither can six other + * `z.lazy` exports of this face: their bodies name the very const being declared + * (or, for `SchemaNodeSchema`, one declared below it), so evaluating the body + * eagerly throws `ReferenceError: Cannot access '' before initialization` + * at module load. The `z.lazy` there is buying a TDZ dodge, not a style. Measured + * one schema at a time in `../__tests__/zod-lazy-getter-identity-7918.test.ts` + * — read that before "fixing" any of them to match this one. */ -export const FilterBuilderConditionSchema: z.ZodType = z.lazy(() => - z.object({ - field: z.string().describe('Field name'), - operator: FilterOperatorSchema.describe('Filter operator'), - value: z.any().optional().describe('Filter value'), - }) -); +const FilterBuilderConditionObject = z.object({ + field: z.string().describe('Field name'), + operator: FilterOperatorSchema.describe('Filter operator'), + value: z.any().optional().describe('Filter value'), +}); + +export const FilterBuilderConditionSchema: z.ZodType = z.lazy(() => FilterBuilderConditionObject); /** * Filter Group Schema — the shape `FilterBuilder` actually reads