diff --git a/.changeset/react-page-lazy-scope-and-runner-memo.md b/.changeset/react-page-lazy-scope-and-runner-memo.md new file mode 100644 index 0000000000..195ca6869c --- /dev/null +++ b/.changeset/react-page-lazy-scope-and-runner-memo.md @@ -0,0 +1,34 @@ +--- +"@object-ui/core": patch +"@object-ui/react-runtime": patch +"@object-ui/components": patch +--- + +fix(sdui): lazily-registered public blocks reach a `kind:'react'` page's scope, and ReactRunner keeps the errors it catches + +Two defects in the trusted `kind:'react'` page tier. + +**objectui#2953 — the contract skipped lazy blocks.** `getPublicConfigs()` +resolved every curated `PUBLIC_BLOCKS` tag through `getConfig()`, which reads +loaded registrations only, so a block registered with `registerLazy()` was +absent from the contract until its plugin chunk happened to be imported. In +`apps/console` that silently dropped `object-kanban`, `object-calendar`, +`object-gantt`, `object-timeline`, `object-map` and `markdown` from every react +page's scope — writing `` threw `ReferenceError` even though the +tag is a first-class contract member, and whether it threw depended on load +order. `getPublicConfigs()` now resolves pending lazy stubs too, returning them +with `lazy: true` and no `component` (new `PublicComponentConfig` type); the +injected wrapper renders through `SchemaRenderer`, which triggers the loader and +shows its placeholder. `getConfig()` stays loaded-only by design. + +**objectui#2954 — ReactRunner discarded its own error state.** +`getDerivedStateFromProps` re-transpiled and re-evaluated the page source on +every render and unconditionally set `error: null`. React runs it before the +re-render that follows `getDerivedStateFromError`, so the boundary threw away +the error it had just caught, rebuilt an identical throwing element, and the +throw escaped past its own `fallback` to the renderer's generic panel; `onError` +was gated on state that had already been cleared and never fired for a +compile-time error at all; and each compile minted a fresh page function — a new +element type — that remounted the subtree and wiped the page's `useState`. The +transpile+eval is now memoised on `(code, scope)`, errors persist until the +inputs actually change, and `onError` reports each error exactly once. diff --git a/packages/components/src/__tests__/react-page-scope.test.tsx b/packages/components/src/__tests__/react-page-scope.test.tsx index 3ccde8bd40..5f31d77510 100644 --- a/packages/components/src/__tests__/react-page-scope.test.tsx +++ b/packages/components/src/__tests__/react-page-scope.test.tsx @@ -16,12 +16,15 @@ * PUBLIC_BLOCKS or flip it to `isContainer` and it silently vanishes from every * react page's scope while type-check, lint and build all stay green. * - * These tests pin the three halves of that contract: + * These tests pin the four halves of that contract: * 1. `list-view` / `object-form` stay eligible for injection; * 2. an author writes `` with FLAT props and no import, and the * wrapper folds them into the block's `schema`; - * 3. an identifier that is genuinely absent fails LOUDLY — the negative - * control that keeps (2) from passing vacuously. + * 3. a block that is only *lazily* registered is in the scope too — the + * contract may not depend on which plugin chunks happen to be loaded + * (objectui#2953); + * 4. an identifier that is genuinely absent fails LOUDLY — the negative + * control that keeps (2) and (3) from passing vacuously. * * Registered stand-ins are used instead of the real plugin-list / plugin-form: * `packages/components` sits BELOW the plugins in the dependency graph, and the @@ -35,7 +38,7 @@ import { ComponentRegistry } from '@object-ui/core'; import { SchemaRenderer, AdapterCtx } from '@object-ui/react'; /** Props the stand-in blocks last received, for flat-prop assertions. */ -const captured: { listView?: any; objectForm?: any } = {}; +const captured: { listView?: any; objectForm?: any; kanban?: any } = {}; const adapter = { find: async () => [], getObjectSchema: async () => ({ name: 'showcase_project', fields: {} }) } as any; @@ -73,6 +76,7 @@ afterAll(() => { beforeEach(() => { captured.listView = undefined; captured.objectForm = undefined; + captured.kanban = undefined; }); // --------------------------------------------------------------------------- @@ -85,7 +89,7 @@ describe('kind:\'react\' scope eligibility', () => { (tag) => { const cfg = ComponentRegistry.getPublicConfigs().find((c: any) => c.type === tag); // Missing from PUBLIC_BLOCKS, or marked isContainer, => dropped from the - // scope of every kind:'react' page (react-page.tsx:56-58). + // scope of every kind:'react' page (react-page.tsx:64-66). expect(cfg).toBeTruthy(); expect((cfg as any).isContainer).toBeFalsy(); }, @@ -112,7 +116,7 @@ function Page() { expect(await findByTestId('list-view-double')).toBeTruthy(); expect(await findByTestId('object-form-double')).toBeTruthy(); // The failure mode this guards: `ReferenceError: ListView is not defined` - // surfaced through ReactRunner's fallback (react-page.tsx:170-175). + // surfaced through ReactRunner's fallback (react-page.tsx:186-191). expect(queryByText('React page error')).toBeNull(); }); @@ -125,7 +129,7 @@ function Page() { await findByTestId('list-view-double'); // Flat JSX props are folded into the schema bag, and the discriminator wins - // the `type` slot (react-page.tsx:71-76). + // the `type` slot (react-page.tsx:79-84). expect(captured.listView.schema).toMatchObject({ type: 'list-view', objectName: 'showcase_project', @@ -136,7 +140,7 @@ function Page() { // apps/console/src/sdui-workbench-preview.tsx is built on onRowClick. expect(typeof captured.listView.onRowClick).toBe('function'); // The wrapper stamps the live adapter in, since list-view reads dataSource - // from props rather than from context (react-page.tsx:52-55). + // from props rather than from context (react-page.tsx:61-63). expect(captured.listView.schema.dataSource).toBe(adapter); }); @@ -152,27 +156,80 @@ function Page() { }); // --------------------------------------------------------------------------- -// 3. Negative control — absence must be loud +// 3. Lazily-registered blocks are contract members too (objectui#2953) +// --------------------------------------------------------------------------- + +describe('kind:\'react\' scope — lazily-registered blocks', () => { + it('injects a curated block that is only registered lazily', async () => { + // How apps/console registers the heavy view plugins: a stub at boot, the + // chunk imported on first use. Six PUBLIC_BLOCKS tags live this way + // (object-kanban / -calendar / -gantt / -timeline / -map / markdown), and + // all six used to be missing from every react page's scope, because + // getPublicConfigs() resolved each curated tag through getConfig(), which + // reads loaded registrations only. + ComponentRegistry.registerLazy( + 'object-kanban', + async () => { + ComponentRegistry.register( + 'object-kanban', + (props: any) => { + captured.kanban = props; + return
; + }, + { namespace: 'plugin-kanban' }, + ); + }, + { namespace: 'plugin-kanban', category: 'view' }, + ); + + try { + // The tag IS in the contract before its chunk has been imported … + const cfg = ComponentRegistry.getPublicConfigs().find((c: any) => c.type === 'object-kanban'); + expect(cfg).toBeTruthy(); + expect((cfg as any).lazy).toBe(true); + + // … so `` resolves in the page source instead of throwing + // `ReferenceError: ObjectKanban is not defined`. The injected wrapper + // defers to SchemaRenderer, which fires the loader, shows the "Loading…" + // placeholder, and re-renders once the plugin registers for real. + const source = ` +function Page() { + return ; +}`; + const { findByTestId, queryByText } = renderReactPage(source); + + expect(await findByTestId('kanban-double')).toBeTruthy(); + expect(queryByText('React page error')).toBeNull(); + expect(captured.kanban.schema).toMatchObject({ type: 'object-kanban', groupBy: 'status' }); + } finally { + ComponentRegistry.unregister('object-kanban', 'plugin-kanban'); + ComponentRegistry.unregister('object-kanban'); + } + }); +}); + +// --------------------------------------------------------------------------- +// 4. Negative control — absence must be loud // --------------------------------------------------------------------------- describe('kind:\'react\' unknown identifier', () => { - it('surfaces the ReferenceError instead of failing silently', async () => { + it('surfaces the ReferenceError in the page-level error panel', async () => { const source = ` function Page() { return ; }`; - const { container } = renderReactPage(source); + const { container, findByText } = renderReactPage(source); // Proves the assertions above are meaningful: when a block really is absent // from scope, the author sees this. // - // Asserted on the message rather than on a specific panel, because the - // catching boundary is NOT the obvious one. ReactRunner is its own error - // boundary, but `getDerivedStateFromProps` re-transpiles on every render - // and unconditionally resets `error: null`, so the recovery render - // re-throws and the error escapes PAST the "React page error" fallback to - // SchemaRenderer's boundary ("Component "home" failed to render"). Pinning - // the panel would pin that quirk; pinning the message pins the contract. + // The panel is ReactRunner's own `fallback` (react-page.tsx:186-191). + // Pinning it also pins objectui#2954: `getDerivedStateFromProps` used to + // re-transpile on every render and reset `error: null`, so the recovery + // render rebuilt the same throwing element and the error escaped PAST this + // fallback to SchemaRenderer's boundary ("Component "home" failed to + // render") — the styled, page-specific panel was unreachable. + expect(await findByText('React page error')).toBeTruthy(); await waitFor(() => expect(container.textContent).toContain('TotallyNotARegisteredBlock is not defined'), ); diff --git a/packages/components/src/renderers/layout/react-page.tsx b/packages/components/src/renderers/layout/react-page.tsx index 2d185d8bae..ac7135854d 100644 --- a/packages/components/src/renderers/layout/react-page.tsx +++ b/packages/components/src/renderers/layout/react-page.tsx @@ -47,6 +47,14 @@ function toPascal(tag: string): string { // data/leaf blocks (non-containers) as prop-driven wrappers; layout containers // are intentionally left out — in react mode the author composes layout with // real HTML + Tailwind, not our schema-children renderers. +// +// Lazily-registered blocks (`object-kanban`, `object-map`, `markdown`, … — see +// apps/console/src/main.tsx) are in here too: `getPublicConfigs()` resolves +// `registerLazy` stubs, so the scope is complete at build time regardless of +// which plugin chunks have been imported (objectui#2953). Each wrapper defers +// to SchemaRenderer, which triggers the loader and renders the placeholder, so +// the scope itself never has to change once built — which is exactly what lets +// it stay identity-stable and keeps the page from remounting (objectui#2954). function buildComponentScope(dataSource: unknown): Record> { const scope: Record> = {}; const seen = new Set(); @@ -123,6 +131,14 @@ export const ReactKindPage: React.FC<{ schema: any }> = ({ schema }) => { }; }, [capabilityEnabled]); + // Keep this identity STABLE. ReactRunner recompiles the page whenever the + // scope identity changes, and every compile mints a fresh `Page` function — + // i.e. a new element type — which remounts the subtree and wipes the page's + // own `useState` (objectui#2954). That's why this deliberately does not + // subscribe to ComponentRegistry changes the way SchemaRenderer does: a lazy + // plugin finishing its registration notifies the registry, and rebuilding the + // scope there would reset every interactive page on the screen. It doesn't + // need to — `buildComponentScope` already sees lazy blocks (objectui#2953). const scope = React.useMemo( () => ({ ...buildComponentScope(adapter), diff --git a/packages/core/src/registry/Registry.ts b/packages/core/src/registry/Registry.ts index a641919eac..a162b3c3d6 100644 --- a/packages/core/src/registry/Registry.ts +++ b/packages/core/src/registry/Registry.ts @@ -66,6 +66,23 @@ export type ComponentConfig = ComponentMeta & { component: ComponentRenderer; }; +/** + * A CONTRACT-surface entry (ADR-0080), as returned by + * {@link Registry.getPublicConfigs}. + * + * Same shape as {@link ComponentConfig} except `component` is absent while the + * entry is still a pending `registerLazy` stub: the plugin module has not been + * imported yet, so there is no renderer to hand out. Consumers render such an + * entry through `SchemaRenderer`, which triggers the loader and shows a + * placeholder in the meantime (objectui#2953). + */ +export type PublicComponentConfig = ComponentMeta & { + type: string; + component?: ComponentRenderer; + /** True while this entry is a `registerLazy` stub whose loader has not run. */ + lazy?: boolean; +}; + /** * Lazy loader function used by `Registry.registerLazy`. The loader is invoked * the first time a missing component type is requested through `getAsync`/the @@ -298,7 +315,14 @@ export class Registry { /** * Get component configuration by type with namespace support. - * + * + * LOADED registrations only — a type that exists solely as a `registerLazy` + * stub returns `undefined` here, because callers read `.component` off the + * result and a stub has no renderer until its loader runs. The *contract* + * question ("is this tag part of the public surface?") must not depend on + * load order, so {@link getPublicConfigs} resolves lazy stubs separately + * (objectui#2953). Use {@link hasLazy} / {@link loadLazy} to reach a stub. + * * @param type - Component type (e.g., 'button' or 'ui:button') * @param namespace - Optional namespace for lookup priority * @returns Component configuration or undefined @@ -349,18 +373,46 @@ export class Registry { return Array.from(this.components.values()); } + /** + * Resolve `tag` for the CONTRACT surface: the loaded registration when the + * component is already in the registry, otherwise the metadata of a pending + * `registerLazy` stub. + * + * A block registered lazily is a first-class member of the contract — the + * loader is recorded at boot and the plugin chunk is imported on first use — + * so contract membership must not hinge on whether that import happened to + * have run yet (objectui#2953). + */ + private getContractConfig(tag: string): PublicComponentConfig | undefined { + const loaded = this.components.get(tag); + if (loaded) return loaded; + const entry = this.lazyEntries.get(tag); + if (!entry) return undefined; + // Mirror registerLazy's key derivation so the canonical `type` matches the + // one `register()` will store once the loader runs — that keeps the dedupe + // below stable across the load. `tag` may already carry the namespace + // (lazyEntries holds both keys; curated tags like `record:details` are + // themselves colon-shaped), so don't prefix it twice. + const ns = entry.meta?.namespace; + const canonical = ns && !tag.startsWith(`${ns}:`) ? `${ns}:${tag}` : tag; + return { ...entry.meta, type: canonical, lazy: true }; + } + /** * Get the curated PUBLIC-tier component configs (ADR-0080) — those registered * with `tier: 'public'`. This is the contract/AI-vocabulary surface, a subset * of the full rendering capability returned by {@link getAllConfigs}. + * + * Includes blocks that are only lazily registered so far; those come back + * with `lazy: true` and no `component` (see {@link PublicComponentConfig}). */ - getPublicConfigs(): ComponentConfig[] { + getPublicConfigs(): PublicComponentConfig[] { // Dedupe by the config's canonical (namespaced) `type` — a component is // registered under both a bare and a namespaced key pointing at the same // canonical type, and we want one contract entry per component. const seenCanonical = new Set(); - const out: ComponentConfig[] = []; - const add = (tag: string, cfg: ComponentConfig | undefined): void => { + const out: PublicComponentConfig[] = []; + const add = (tag: string, cfg: PublicComponentConfig | undefined): void => { if (!cfg || seenCanonical.has(cfg.type)) return; seenCanonical.add(cfg.type); // The contract surface is keyed by the bare/curated tag authors write, @@ -368,11 +420,15 @@ export class Registry { out.push({ ...cfg, type: tag }); }; // Curated contract list first (stable, reviewable order) … - for (const tag of PUBLIC_BLOCKS) add(tag, this.getConfig(tag)); + for (const tag of PUBLIC_BLOCKS) add(tag, this.getContractConfig(tag)); // … plus any bare registration that opted in explicitly via `tier: 'public'`. for (const [key, cfg] of this.components.entries()) { if (cfg.tier === 'public' && !key.includes(':')) add(key, cfg); } + // … and the same opt-in for stubs whose loader has not run yet. + for (const [key, entry] of this.lazyEntries.entries()) { + if (entry.meta?.tier === 'public' && !key.includes(':')) add(key, this.getContractConfig(key)); + } return out; } diff --git a/packages/core/src/registry/__tests__/public-tier.test.ts b/packages/core/src/registry/__tests__/public-tier.test.ts index 54ff0704c4..c77db5430d 100644 --- a/packages/core/src/registry/__tests__/public-tier.test.ts +++ b/packages/core/src/registry/__tests__/public-tier.test.ts @@ -33,3 +33,82 @@ describe('getPublicConfigs — ADR-0080 curated public tier (capability ≠ cont expect(PUBLIC_BLOCK_SET.size).toBe(PUBLIC_BLOCKS.length); }); }); + +// --------------------------------------------------------------------------- +// Lazy registrations are contract members too (objectui#2953) +// --------------------------------------------------------------------------- + +describe('getPublicConfigs — lazy registrations (objectui#2953)', () => { + const loader = () => Promise.resolve(); + + it('includes a curated block that is only lazily registered', () => { + const r = new Registry(); + r.registerLazy('object-kanban', loader, { namespace: 'plugin-kanban', category: 'view' }); + + const cfg = r.getPublicConfigs().find((c) => c.type === 'object-kanban'); + // Before the fix this was `undefined`, because getPublicConfigs() resolved + // each curated tag through getConfig(), which only reads loaded + // registrations — so every lazily-registered public block silently fell + // out of the contract (and out of every kind:'react' page's scope). + expect(cfg).toBeTruthy(); + expect(cfg!.lazy).toBe(true); + expect(cfg!.component).toBeUndefined(); + // Metadata comes from the stub, so consumers can still filter on it. + expect(cfg!.namespace).toBe('plugin-kanban'); + expect(cfg!.isContainer).toBeFalsy(); + }); + + it('does not depend on load order — same entry before and after the load', () => { + const r = new Registry(); + r.registerLazy('object-map', loader, { namespace: 'plugin-map' }); + const before = r.getPublicConfigs().map((c) => c.type); + + // The plugin chunk lands: its module registers the real component. + r.register('object-map', C, { namespace: 'plugin-map' }); + const after = r.getPublicConfigs(); + + expect(before).toEqual(['object-map']); + expect(after.map((c) => c.type)).toEqual(['object-map']); + // …and now it is a fully loaded entry, not a stub. + expect(after[0].lazy).toBeFalsy(); + expect(after[0].component).toBe(C); + }); + + it('emits one entry per component when a stub is registered under both keys', () => { + const r = new Registry(); + // registerLazy stores the entry under BOTH `plugin-kanban:object-kanban` + // and the bare `object-kanban`; the contract wants one row, keyed bare. + r.registerLazy('object-kanban', loader, { namespace: 'plugin-kanban' }); + r.registerLazy('kanban', loader, { namespace: 'view' }); // not curated + + const types = r.getPublicConfigs().map((c) => c.type); + expect(types).toEqual(['object-kanban']); + }); + + it('honours a colon-shaped curated tag without double-prefixing its namespace', () => { + const r = new Registry(); + r.registerLazy('record:details', loader, { namespace: 'record' }); + + const types = r.getPublicConfigs().map((c) => c.type); + expect(types).toEqual(['record:details']); + }); + + it('surfaces a lazy `tier:public` opt-in outside the curated list', () => { + const r = new Registry(); + r.registerLazy('my-widget', loader, { namespace: 'x', tier: 'public' }); + r.registerLazy('my-internal', loader, { namespace: 'x' }); + + const types = r.getPublicConfigs().map((c) => c.type); + expect(types).toEqual(['my-widget']); + }); + + it('leaves getConfig() loaded-only — a stub is not a renderable config', () => { + const r = new Registry(); + r.registerLazy('object-gantt', loader, { namespace: 'plugin-gantt' }); + + // Callers of getConfig() read `.component`; handing them a stub would give + // them `undefined` to render. They go through hasLazy()/loadLazy() instead. + expect(r.getConfig('object-gantt')).toBeUndefined(); + expect(r.hasLazy('object-gantt')).toBe(true); + }); +}); diff --git a/packages/react-runtime/src/__tests__/ReactRunner.test.tsx b/packages/react-runtime/src/__tests__/ReactRunner.test.tsx new file mode 100644 index 0000000000..cf54990761 --- /dev/null +++ b/packages/react-runtime/src/__tests__/ReactRunner.test.tsx @@ -0,0 +1,198 @@ +/** + * ObjectUI + * Copyright (c) 2024-present ObjectStack Inc. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * `ReactRunner` — the trusted `kind:'react'` execution tier's error boundary + * and compile cache (objectui#2954). + * + * `getDerivedStateFromProps` used to transpile + eval the page source on EVERY + * render and unconditionally set `error: null`. React runs it before the + * re-render that follows `getDerivedStateFromError`, so the boundary threw away + * the error it had just caught, rebuilt an identical throwing element, and the + * throw escaped past its own `fallback` — plus each compile minted a fresh + * `Page` function, i.e. a new element type, remounting the page and wiping its + * state. + * + * None of that is visible to type-check, lint or build, and the failure is + * silent-ish (the page still renders, just via somebody else's error panel and + * with its state reset), so it is pinned here. + */ + +import { describe, it, expect, vi } from 'vitest'; +import { render, fireEvent } from '@testing-library/react'; +import React from 'react'; +import { ReactRunner, generateElement } from '../index.js'; + +/** Module-scope so the identity stays stable across re-renders — the runner + * recompiles on `scope` identity change, which is the point of several tests. */ +const EMPTY_SCOPE = {}; + +const fallback = (error: Error) =>
{String(error)}
; + +const COUNTER_SOURCE = ` +export default function Page() { + const [n, setN] = React.useState(0); + return ; +}`; + +// --------------------------------------------------------------------------- +// Baseline +// --------------------------------------------------------------------------- + +describe('generateElement', () => { + it('compiles a bare JSX fragment into an element', () => { + const el = generateElement('

hi

'); + expect(React.isValidElement(el)).toBe(true); + }); + + it('returns null for empty source', () => { + expect(generateElement(' ')).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 1. The boundary must actually hold its own errors +// --------------------------------------------------------------------------- + +describe('ReactRunner error boundary', () => { + it('renders its own fallback for an error thrown while RENDERING the page', () => { + const { queryByTestId } = render( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} />, + ); + + // The regression: `getDerivedStateFromProps` cleared the error before the + // recovery render, the same element was rebuilt, and the ReferenceError + // escaped to whatever boundary sat ABOVE the runner — so `fallback` was + // dead code for every render-phase error. + const panel = queryByTestId('runner-fallback'); + expect(panel).toBeTruthy(); + expect(panel!.textContent).toContain('NotInScope is not defined'); + }); + + it('renders its own fallback for an error thrown while COMPILING the page', () => { + const { queryByTestId } = render( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} />, + ); + expect(queryByTestId('runner-fallback')).toBeTruthy(); + }); + + it('recovers when the code changes to something that works', () => { + const { queryByTestId, rerender } = render( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} />, + ); + expect(queryByTestId('runner-fallback')).toBeTruthy(); + + // Holding the error must not make it sticky: new inputs => fresh compile. + rerender( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} />, + ); + expect(queryByTestId('runner-fallback')).toBeNull(); + expect(queryByTestId('ok')).toBeTruthy(); + }); + + it('reports a render-phase error through onError exactly once', () => { + const onError = vi.fn(); + const { rerender } = render( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} onError={onError} />, + ); + // `componentDidUpdate` gated on an `error` the next + // `getDerivedStateFromProps` had already cleared, so this used to be 0. + expect(onError).toHaveBeenCalledTimes(1); + expect(String(onError.mock.calls[0][0])).toContain('NotInScope is not defined'); + + // Held state must not re-report on every subsequent render. + rerender( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} onError={onError} />, + ); + expect(onError).toHaveBeenCalledTimes(1); + }); + + it('reports a compile-time error through onError on mount', () => { + const onError = vi.fn(); + // Only `componentDidUpdate` reported errors, and it does not run for the + // first render — so an eval error at mount was never reported at all. + render( + ; }`} scope={EMPTY_SCOPE} fallback={fallback} onError={onError} />, + ); + expect(onError).toHaveBeenCalledTimes(1); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Compile once per (code, scope) — not once per render +// --------------------------------------------------------------------------- + +describe('ReactRunner compile memoisation', () => { + it('does not recompile when re-rendered with the same code and scope', () => { + const compiled = vi.fn(); + const scope = { compiled }; + const source = ` +compiled(); +export default function Page() { return ; }`; + + const { rerender, queryByTestId } = render(); + expect(queryByTestId('probe')).toBeTruthy(); + expect(compiled).toHaveBeenCalledTimes(1); + + rerender(); + rerender(); + expect(compiled).toHaveBeenCalledTimes(1); + }); + + it('recompiles when the code changes', () => { + const compiled = vi.fn(); + const scope = { compiled }; + const mk = (label: string) => ` +compiled(); +export default function Page() { return ${label}; }`; + + const { rerender, getByTestId } = render(); + rerender(); + + expect(compiled).toHaveBeenCalledTimes(2); + expect(getByTestId('probe').textContent).toBe('b'); + }); + + it('recompiles when the scope identity changes', () => { + const compiled = vi.fn(); + const source = ` +compiled(); +export default function Page() { return ; }`; + + const { rerender } = render(); + rerender(); + expect(compiled).toHaveBeenCalledTimes(2); + }); + + it('keeps the page mounted — its useState survives a parent re-render', () => { + function Host({ tick }: { tick: number }) { + return ( + <> + {tick} + + + ); + } + + const { getByTestId, rerender } = render(); + fireEvent.click(getByTestId('counter')); + expect(getByTestId('counter').textContent).toBe('1'); + + rerender(); + + // Every compile produces a new `Page` function — a new element TYPE — so + // recompiling per render remounted the subtree and reset this to '0'. + expect(getByTestId('tick').textContent).toBe('1'); + expect(getByTestId('counter').textContent).toBe('1'); + }); + + it('keeps the page mounted across its OWN state updates', () => { + const { getByTestId } = render(); + fireEvent.click(getByTestId('counter')); + fireEvent.click(getByTestId('counter')); + expect(getByTestId('counter').textContent).toBe('2'); + }); +}); diff --git a/packages/react-runtime/src/index.tsx b/packages/react-runtime/src/index.tsx index 3f4cbf0114..6e0365d348 100644 --- a/packages/react-runtime/src/index.tsx +++ b/packages/react-runtime/src/index.tsx @@ -57,28 +57,60 @@ export interface ReactRunnerProps { onError?: (error: Error) => void; } +/** Marks "nothing compiled yet" — distinct from any real `code`/`scope` value. */ +const UNSET = Symbol('unset'); + interface ReactRunnerState { element: ReactElement | null; error: Error | null; + /** The `code` the current `element`/`error` was produced from. */ + compiledCode: string | typeof UNSET; + /** The `scope` (by identity) the current `element`/`error` was produced from. */ + compiledScope: Scope | undefined | typeof UNSET; } /** Renders a JSX/TSX source string with a built-in error boundary. */ export class ReactRunner extends Component { - state: ReactRunnerState = { element: null, error: null }; + state: ReactRunnerState = { element: null, error: null, compiledCode: UNSET, compiledScope: UNSET }; - static getDerivedStateFromProps(props: ReactRunnerProps): Partial | null { + /** + * Transpile + eval ONLY when `code` or the `scope` identity actually changed. + * + * Recompiling on every render broke this boundary three ways (objectui#2954): + * 1. React runs this before the re-render that follows + * `getDerivedStateFromError`, and the old body unconditionally set + * `error: null` — so the error just caught was discarded, an identical + * throwing element was rebuilt, and the throw escaped PAST our own + * `fallback` to whatever boundary sits above us. + * 2. `onError` was gated on an `error` that this had already cleared. + * 3. Every eval mints a fresh `Page` function, so a new element *type* on + * each render remounts the page subtree and wipes its `useState`. + */ + static getDerivedStateFromProps( + props: ReactRunnerProps, + state: ReactRunnerState, + ): Partial | null { + if (props.code === state.compiledCode && props.scope === state.compiledScope) return null; + const compiled = { compiledCode: props.code, compiledScope: props.scope }; try { - return { element: generateElement(props.code, props.scope), error: null }; + return { ...compiled, element: generateElement(props.code, props.scope), error: null }; } catch (error) { - return { element: null, error: error as Error }; + return { ...compiled, element: null, error: error as Error }; } } static getDerivedStateFromError(error: Error): Partial { return { error }; } - componentDidUpdate(): void { + componentDidMount(): void { + // A transpile/eval error is already in state by the time we mount, and + // `componentDidUpdate` never runs for the first render. if (this.state.error) this.props.onError?.(this.state.error); } + componentDidUpdate(_prevProps: ReactRunnerProps, prevState: ReactRunnerState): void { + // Report each error once, on the transition — the state now persists, so + // firing on every subsequent render would repeat the same one forever. + if (this.state.error && this.state.error !== prevState.error) this.props.onError?.(this.state.error); + } render(): ReactNode { if (this.state.error) { return this.props.fallback