Add shared renderer fallback mechanism: web_core contract + React implementation (#2013)#2088
Add shared renderer fallback mechanism: web_core contract + React implementation (#2013)#2088matthewvilaysack wants to merge 8 commits into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
|
@googlebot I signed it! |
There was a problem hiding this comment.
Code Review
This pull request introduces a shared fallback contract (A2uiFallbackState, A2uiFallbackInfo) in web_core and implements it in the React renderer via a new FallbackContext to handle loading and unknownComponent states consistently. By default, pending or unknown components now render nothing, with unknown types logging a warning and dispatching a COMPONENT_NOT_FOUND error. The reviewer feedback highlights several important React best practices: moving side effects (warnings and error dispatches) from the render phase into a useEffect to maintain render purity, lazily initializing the warnedTypesRef to avoid redundant Set allocations on every render, and defensively memoizing the fallbacks prop in A2uiSurface to prevent unnecessary re-renders when inline object literals are used.
| const componentModel = surface.componentsModel.get(id); | ||
|
|
||
| if (!componentModel) { | ||
| return <div style={{color: 'gray', padding: '4px'}}>[Loading {id}...]</div>; | ||
| return renderFallback(fallbacks?.loading, { | ||
| state: 'loading', | ||
| componentId: id, | ||
| ...(parentComponentType !== undefined ? {parentComponentType} : {}), | ||
| }); | ||
| } | ||
|
|
||
| const compImpl = surface.catalog.components.get(componentModel.type); | ||
|
|
||
| if (!compImpl) { | ||
| return <div style={{color: 'red'}}>Unknown component: {componentModel.type}</div>; | ||
| if (!warnedTypesRef.current.has(componentModel.type)) { | ||
| warnedTypesRef.current.add(componentModel.type); | ||
| console.warn(`Component implementation not found for type: ${componentModel.type}`); | ||
| // Deferred: onError listeners may setState, which React forbids while | ||
| // another component is rendering — so nothing may run them synchronously | ||
| // from this render body. | ||
| queueMicrotask(() => { | ||
| void surface.dispatchError({ | ||
| code: 'COMPONENT_NOT_FOUND', | ||
| message: `Component implementation not found for type: ${componentModel.type}`, | ||
| componentId: id, | ||
| componentType: componentModel.type, | ||
| }); | ||
| }); | ||
| } | ||
| return renderFallback(fallbacks?.unknownComponent, { | ||
| state: 'unknownComponent', | ||
| componentId: id, | ||
| componentType: componentModel.type, | ||
| ...(parentComponentType !== undefined ? {parentComponentType} : {}), | ||
| }); | ||
| } |
There was a problem hiding this comment.
Performing side effects (such as logging warnings with console.warn and dispatching errors via surface.dispatchError) directly inside the render function is a violation of React's pure render contract.
Even though queueMicrotask defers the execution to avoid the "Cannot update a component while rendering a different component" warning, scheduling a microtask during render is still a side effect. If React aborts or retries the render (which is common in Concurrent Mode, Suspense, or during error recovery), the microtask will still execute, leading to duplicate warnings or phantom error dispatches for components that were never actually committed to the DOM.
Additionally, if the surface prop changes, the warnedTypesRef is not cleared, which means the new surface will not report errors for any unknown component types that were already warned about on the previous surface.
Solution:
- Move these side effects into a
useEffect. SinceuseEffectruns after the render is committed to the screen, it is completely safe to trigger state updates (such as those inonErrorlisteners) without any warnings, eliminating the need forqueueMicrotaskentirely. - Clear the
warnedTypesRefwhen thesurfaceinstance changes to ensure correct error reporting across surface transitions.
const componentModel = surface.componentsModel.get(id);
const compImpl = componentModel ? surface.catalog.components.get(componentModel.type) : undefined;
const missingType = componentModel && !compImpl ? componentModel.type : null;
// Reset warned types if the surface instance changes
const lastSurfaceRef = React.useRef(surface);
if (lastSurfaceRef.current !== surface) {
lastSurfaceRef.current = surface;
if (warnedTypesRef.current) {
warnedTypesRef.current.clear();
}
}
React.useEffect(() => {
if (missingType) {
if (warnedTypesRef.current && !warnedTypesRef.current.has(missingType)) {
warnedTypesRef.current.add(missingType);
console.warn(`Component implementation not found for type: ${missingType}`);
void surface.dispatchError({
code: 'COMPONENT_NOT_FOUND',
message: `Component implementation not found for type: ${missingType}`,
componentId: id,
componentType: missingType,
});
}
}
}, [missingType, surface, id]);
if (!componentModel) {
return renderFallback(fallbacks?.loading, {
state: 'loading',
componentId: id,
...(parentComponentType !== undefined ? {parentComponentType} : {}),
});
}
if (!compImpl) {
return renderFallback(fallbacks?.unknownComponent, {
state: 'unknownComponent',
componentId: id,
componentType: componentModel.type,
...(parentComponentType !== undefined ? {parentComponentType} : {}),
});
}
| // Tracks every unknown type warned about, so re-renders (including | ||
| // StrictMode double-invokes) and type flaps (Nope -> Nope2 -> Nope) do | ||
| // not repeat the warning for this mount. | ||
| const warnedTypesRef = useRef<Set<string>>(new Set()); |
There was a problem hiding this comment.
Initializing useRef with new Set() directly inside the render body causes a new Set instance to be created on every single render of DeferredChild, even though React only uses the initial value on the first render.
To avoid this redundant object creation and save memory/CPU cycles, you can initialize the ref lazily.
const warnedTypesRef = useRef<Set<string>>();
if (!warnedTypesRef.current) {
warnedTypesRef.current = new Set();
}
| }> = ({surface, fallbacks}) => { | ||
| // The root component always has ID 'root' and base path '/' | ||
| return <DeferredChild surface={surface} id="root" basePath="/" />; | ||
| return ( | ||
| <FallbackContext.Provider value={fallbacks}> | ||
| <DeferredChild surface={surface} id="root" basePath="/" /> | ||
| </FallbackContext.Provider> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
While the documentation correctly warns consumers to hoist or memoize the fallbacks object, developers frequently pass inline object literals (e.g., fallbacks={{ loading: <Spinner /> }}). This creates a new object identity on every render, causing all nested DeferredChild components to bypass memoization and re-render.
We can defensively protect against this inside A2uiSurface by memoizing the fallbacks object based on its individual properties.
| }> = ({surface, fallbacks}) => { | |
| // The root component always has ID 'root' and base path '/' | |
| return <DeferredChild surface={surface} id="root" basePath="/" />; | |
| return ( | |
| <FallbackContext.Provider value={fallbacks}> | |
| <DeferredChild surface={surface} id="root" basePath="/" /> | |
| </FallbackContext.Provider> | |
| ); | |
| }; | |
| }> = ({surface, fallbacks}) => { | |
| const memoizedFallbacks = useMemo( | |
| () => fallbacks, | |
| [fallbacks?.loading, fallbacks?.unknownComponent], | |
| ); | |
| // The root component always has ID 'root' and base path '/' | |
| return ( | |
| <FallbackContext.Provider value={memoizedFallbacks}> | |
| <DeferredChild surface={surface} id="root" basePath="/" /> | |
| </FallbackContext.Provider> | |
| ); | |
| }; |
9e2c9ec to
5160124
Compare
|
@googlebot I signed it! |
1 similar comment
|
@googlebot I signed it! |
5160124 to
0ff3aca
Compare
Record in a2ui-findings §7 that a2ui-project/a2ui#2088 adds a shared renderer fallback mechanism upstream; once it ships in an official @a2ui/react release, drop our local yarn patch in favor of it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
0ff3aca to
d8fda9d
Compare
|
Added the Angular renderer. Same idea as React underneath. |
New module of type declarations in web_core v0_9 for the shared fallback contract. A2uiFallbackState is a discriminated union of loading, unknownComponent, and error. A2uiFallbackInfo carries componentId, an optional componentType, an optional parentComponentType, and a reason for the error state. Exported from the v0_9 barrel. Additive and does not change existing behavior.
…rer (a2ui-project#2013) The React renderer renders nothing for the loading and unknownComponent states instead of debug placeholders, and keeps a console.warn for unknown types. Consumers can pass an optional fallbacks prop on A2uiSurface, either a node or a render function per state, delivered through context so nested DeferredChild children receive it. Unknown component types also dispatch surface.dispatchError with COMPONENT_NOT_FOUND once per distinct type. The warn and dispatch run in a useEffect so render stays pure. Adds the fallback behavior test matrix and updates the placeholder assertion in adapter.test.tsx.
SurfaceComponent gains optional loadingTemplate and unknownComponentTemplate TemplateRef inputs, rendered through ngTemplateOutlet with component context. An optional A2UI_FALLBACK_TEMPLATES injection token, mirroring A2UI_RENDERER_CONFIG, propagates the templates to nested component hosts. Defaults are unchanged, nothing renders and the existing console warnings still fire, and unknown component types dispatch COMPONENT_NOT_FOUND to match the other renderers. Adds the Karma spec matrix.
|
Thanks for the review. Moved the warn and dispatch into a useEffect so render stays pure, and made the warned-types Set lazy so it is not reallocated on each render. Both are in the latest push. On memoizing the fallbacks prop, I left that to the consumer on purpose. It is documented to be hoisted or memoized like any context value or render prop, and I would rather keep that explicit than hide it behind an internal memo. |
d8fda9d to
f5dcfe0
Compare
…2ui-project#2013) Adds the shared fallback wiring to the Lit renderer. A new fallbacks context is provided by a2ui-surface and threaded through renderA2uiNode, the visible Loading surface default is removed while the loading slot is preserved for consumer projection, and unknown component types dispatch COMPONENT_NOT_FOUND deferred out of render and deduped per surface. The root render error path dispatches an error state to the surface while keeping its visible message.
…a2ui-project#2013) A nested child referenced before it streams no longer throws from ComponentContext construction. The element guards the child id, renders the loading fallback or nothing, and re-renders when the component arrives via a targeted onCreated subscription that is cleaned up on disconnect and when the surface context changes. The element consumes the fallbacks context and threads the parent component type to fallback info.
Converts the legacy Loading surface assertions and adds a matrix covering render-nothing defaults, consumer fallbacks including nested delivery via context, dispatch dedup, subscription cleanup, and cross-surface isolation. Adds the Unreleased changelog entry.
|
Added the Lit renderer, so all four packages are now in: web_core, React, Angular, and Lit. Lit keeps the loading slot but drops the visible default text, guards the nested pending path that used to throw and re-renders when the child arrives, and delivers consumer fallbacks through @lit/context so nested elements receive them. Same COMPONENT_NOT_FOUND dispatch as the other renderers, deferred out of render. The cross renderer test matrix is complete now. Ready for review whenever you have a chance. |
Description
The React renderer shows developer debug placeholders (
[Loading {id}...]divs and redUnknown component:text) to end users. This PR adds the shared fallback mechanism designed and agreed on in #2013: a small types-only contract in web_core, plus all three web renderers (React, Angular, Lit) implementing it. Fixes #2013.Per the discussion with @ditman, this PR grew renderer by renderer so review could start early. All four packages are in now: web_core, React, Angular, and Lit, each following the shared contract with its own idiomatic delivery mechanism. Docs and the framework adapter blueprint are included too.
One note on how the contract evolved during review: the design comment sketched a single info interface with two states, and the landed version is a three arm discriminated union that adds an
errorstate and an optionalparentComponentType, per your feedback on the issue. The state names and literals are unchanged.What is in so far:
src/v0_9/rendering/fallback.ts, a types-only discriminated union with three states (loading,unknownComponent,error), an info payload carryingcomponentId, optionalcomponentTypeandparentComponentType, andreasonfor the error state. Exported from the v0_9 barrel, additive and non-breaking (verified: existing build and all 271 web_core tests pass unchanged, and the react package typechecks against the new types with zero edits to its own declarations).console.warn(same message text as the Lit renderer), and adds an optionalfallbacksprop onA2uiSurface(ReactNodeor(info) => ReactNodeper state) delivered through a context so nestedDeferredChildchildren receive it. TheunknownComponentpath also dispatchessurface.dispatchError({code: 'COMPONENT_NOT_FOUND', message, componentId, componentType})once per distinct unknown type, deferred to a microtask so anonErrorlistener can safely set state.loadingTemplateandunknownComponentTemplateTemplateRefinputs onSurfaceComponent, rendered throughngTemplateOutletwith component context. An optionalA2UI_FALLBACK_TEMPLATESinjection token, mirroringA2UI_RENDERER_CONFIG, carries the templates to nested component hosts. TheunknownComponentpath dispatches the sameCOMPONENT_NOT_FOUNDas React.Loading surface...default is removed while the<slot name="loading">is preserved for consumer projection. A nested child referenced before it streams no longer throws; it renders the fallback or nothing and re-renders when the component arrives via a targeted subscription that is cleaned up on disconnect and surface change. Consumer fallbacks are delivered through@lit/contextso nested elements receive them, and theunknownComponentand error paths dispatch through the same shared channel, deferred out of render.To be explicit about the behavior change: removing the visible React debug divs is deliberate and is the fix this issue asks for. It is a rendering default change, not an API break; every existing prop and export keeps working, and consumers who want visible fallbacks get them via the new
fallbacksprop. I know a v1.0 restructure is in flight; this diff is small and additive on the v0_9 surface, and I am happy to rebase or adapt it if the restructure moves these files.Tests: each renderer adds a matrix asserting render-nothing defaults for both states and consumer-provided fallbacks including nested delivery, plus dispatch dedup and, where relevant, subscription cleanup and cross-surface isolation. The six legacy placeholder-text assertions across the renderers are updated. React suite 462 passing, Angular suite 269 passing, Lit suite 109 passing, web_core suite 271 passing.
Docs: each renderer README gains a copy-paste custom-fallback example, and
blueprints/modules/a2ui_framework_adapter.blueprint.mddocuments the shared fallback contract so other renderers can adopt it.Pre-launch Checklist
One time:
For this PR:
(The diff is web only, so
e2e_test.sh's Flutter path is not exercised; the four web packages build and their test suites pass locally.)If you need help, consider asking for advice on the discussion board.