Node layer for web_core + React - #2077
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces an experimental React surface renderer (A2uiNodeSurface) driven by a new node layer (NodeResolver and ComponentNode) that resolves component references and templates into a live tree of nodes for fine-grained re-renders. It also updates the Catalog interface to distinguish schema-only catalogs from those with executable function implementations, and adds comprehensive unit tests. The review feedback highlights several key improvement opportunities, including restricting object traversal in toViewValue to plain objects to prevent breaking types like Date or Map, unwrapping ZodEffects in schema parsing to support refined schemas, removing a redundant synchronous onChange call to avoid duplicate React state updates, using public Zod APIs instead of internal _def properties, and cleaning up empty sets in pendingParents to prevent minor memory leaks.
jacobsimionato
left a comment
jacobsimionato
left a comment
There was a problem hiding this comment.
This is great! Neat way to prototype it!
| } | ||
|
|
||
| get isPlaceholder(): boolean { | ||
| return this.type === PLACEHOLDER_TYPE; |
There was a problem hiding this comment.
What does this mean again? Could you add a doc comment about it?
| * come from the resolver's cache; untouched arrays keep their identity), so | ||
| * shallow comparison is exact rather than heuristic. | ||
| */ | ||
| setProps(next: TProps): void { |
There was a problem hiding this comment.
I think we don't want application code to be able to call this. Can we provide a readonly public interface for Node, so that the NodeResolver has write access, but the rest of the application code can only read values and update the data model via the bindings in the node, rather than the node props directly?
There was a problem hiding this comment.
Done in 160a82b. ComponentNode is now read-only with a protected constructor: identity fields, props, onDestroyed, addCleanup, toJSON. setProps, dispose are moved to MutableComponentNode, which only the resolver uses and the package does not export, so application code reads props and writes data only through a binding's set.
There is a caveat here in that node.props is still a Signal, so the low-level setValue(node.props, ...) remains a bypass for a determined client. We would need to add a read-only signal type to close that. I haven't explored this, but I'm assuming it would be a breaking change.
| * unchanged props keep reference identity across rebuilds. Child | ||
| * `ComponentNode`s and action closures compare by identity. | ||
| */ | ||
| function stabilize(prev: unknown, next: unknown): unknown { |
There was a problem hiding this comment.
Can this be private?
| * Use in place of a bare string id so the node layer can resolve the child. | ||
| */ | ||
| export function componentReference(): z.ZodTypeAny { | ||
| return ComponentIdSchema.describe(`${SINGLE_REF_MARKER}|${ComponentIdSchema.description ?? ''}`); |
There was a problem hiding this comment.
We already do a hack like this in our schemas here:
We needed it so that when we convert an in-memory Zod catalog to a wire-format catalog, we can replace the subschemas for DynamicString etc with references to common_types.json.
Seems like you needed basically the same thing here - to be able to identify specific special subschemas to understand the meaning of different strings etc.
Could you use the same magic references, e.g. centralize some definition in common-types?
BTW I wrote a proposal at go/a2ui-catalogs-without-json-schema to design our own schema format that would make this less hacky. I'm not a fan of this description hack, but I don't know a way around it without abandoning JSON schema!
There was a problem hiding this comment.
Done in 58c3036. Deleted my A2UI_COMPONENT_REF marker and switched to the same magic references you linked--the node layer now finds child refs by the REF:common_types.json#/... descriptions, so components just declare them with ComponentIdSchema / ChildListSchema. I didn't do that originally because .describe() at a call site wipes the whole description, magic reference included. That's also why generated capabilities were losing their $refs (#1528). Added componentIdWithDescription / childListWithDescription that append instead of replace, moved the basic catalog onto them, and added a test that the generated $refs persist.
| /** | ||
| * One resolved component instance in the rendered tree. | ||
| * | ||
| * A node's `props` hold fully resolved values: primitives for dynamic values, |
There was a problem hiding this comment.
Can we add some way to update the value of two-way bound variables from the node? E.g. for ChoicePicker, it should be possible to somehow update the bound "value" property of the node. I'm not sure the best way to do this. Some ideas:
A. props['value'] returns a BoundValue object which exposes get and set. If we do this, then I think we should probably always return a BoundValue for properties that are DynamicValues etc in the schema, even if they happen to be populated with a static value at runtime. That way, component implementations can always access the value in the same way e.g. props['value'].get() and props['value'].set('new value').
B. Expose setters as additional keys in props e.g. props['setValue'](updatedValue). I think we basically did this in the generic binder.
C. Or expose some other parallel map of setters that is separate to props. E.g. boundValues['value']('updated value').
Vercel did something between A and C I think: https://json-render.dev/docs/registry#component-props
There was a problem hiding this comment.
Done in 3135360. Every dynamic property now resolves to a ResolvedBinding in props, shaped { value, set? }, which is your option A with two changes. First, set is absent when the payload supplied a literal or a function call: there's nothing to write to, and the old synthesized setters silently swallowed those writes, so now an unchecked write doesn't compile.
Second, value is a snapshot rather than a get() closure. A live get() reads the data model at whatever moment the component happens to look, so under React's concurrent rendering two components can disagree about one value within a single frame; that's the tearing problem useSyncExternalStore exists to prevent, and the node surface already uses it for rootNode. A snapshot can't disagree with the emission that delivered it, and the same property means serialized trees and headless reads always describe a state that actually existed. It also keeps toJSON useful: a snapshot serializes as the actual value, while a closure can't appear in JSON at all, so it would have to serialize as a stand-in string the way action closures come out as '<Action>', and serialized trees would lose their two-way values. Writes go through the node's data scope, so a component inside a template row writes to that row's item without knowing it's templated.
You remembered right that option B is what the generic binder already does: it puts extra setValue-style keys in props. That code is untouched in this PR, so nothing views consume changes shape; bindings only exist in node props, which are new here. I didn't want option B for node props because the set<Name> keys can only be typed with mapped types, which no other target language has. Moving the binder itself onto bindings would break A2uiSurface's view contract, so that waits for a major version.
I also read the json-render docs you linked, and you had it right, it's between A and C. They put the current value in props as a plain value and the state path it was bound to in a separate bindings map, and their useBoundProp hook combines the two into a [value, setValue] pair, same shape as useState, with the setter writing to that path; when the prop was a literal there's no path, and the setter it hands back does nothing. That's a fine deal in an all-React library, because a React input displays whatever its state variable holds: if the setter does nothing, the variable never changes, the input visibly freezes, and the developer notices immediately. It doesn't carry over to us for two reasons. Flutter disables its selection controls by passing onChanged: null (TextField does it with enabled: false, decided the same way), so a view has to be able to tell "no write destination" apart from "has one", and a no-op function looks identical to a real one; an absent set is the signal a view can map onto that (and headless consumers render nothing that could visibly freeze). And the [value, setValue] hook idiom is React-specific; Dart and Swift have nothing like it, while a bundled value-plus-set object works as-is in every language I'm aware of.
| * without `render`'s own binder. Absent on binderless implementations, | ||
| * which the node surface falls back to rendering through `render`. | ||
| */ | ||
| view?: React.FC<ReactA2uiComponentProps<any>>; |
There was a problem hiding this comment.
Would it make sense for this to just accept a node and buildChild as the parameter? Then we can have extra sugar in createComponentImplementation that exposes props, in a typesafe way?
There was a problem hiding this comment.
Yes, I made this change in cf09709. view now takes {node, buildChild}, and both factories generate it. The generated wrapper subscribes to the node's props, converts them to the ReactA2uiComponentProps shape authors already write against, and resolves child ids through its own index. The author's function stays typed per component through ResolveA2uiProps, so the typesafe part of your suggestion is covered. Component code is unchanged, and the node never reaches it. A2uiNodeSurface is left as a dispatcher, plus a fallback that renders from raw definitions when an implementation has no view.
One can still write a view by hand without the factories, and that one takes the node directly. That's fine now that nodes are read-only. I exported useSignalValue in 6e4f06e so that's actually doable from outside the package.
Down the road I want a mapped type where dynamic props come through as ResolvedBinding<T>, so views consume bindings directly and the wrapper stops converting. I didn't add it here because nothing would use it yet. The components that replace the current catalog are the natural first users. Happy to go deeper on any of this.
… in types Catalog and SurfaceModel gain a function-kind type parameter defaulting to FunctionImplementation, so existing code is unchanged. A schema-only catalog types its functions as FunctionApi (signatures without code), matching the Python SDK's two-parameter Catalog. The catalog invoker reports a schema-only function as an expression error instead of failing on a missing execute method.
NodeResolver turns a surface's flat component map into a live tree of resolved ComponentNodes exposed through a reactive rootNode. Child references resolve to node objects, ChildList templates spawn one node per array item with per-position reuse, missing components render as placeholder nodes that are upgraded in place, and disposal follows parent-scoped ownership. Malformed payloads degrade safely: cyclic references and unknown catalog types render as placeholders with dispatched errors. Construction requires a catalog typed with FunctionImplementation, so resolving a tree without executable functions is a compile error. A node's props signal emits only when its own resolved properties change; child-internal changes stay on the child's signal. Action context resolves at dispatch time. The test suite ports test_node_graph.py from the Python SDK and adds coverage for late action resolution, shared-child disposal, template key stability, cycles, and emission granularity.
A2uiNodeSurface renders a surface from a NodeResolver instead of the
per-component id recursion in A2uiSurface. One resolver per surface
owns binding and tree maintenance; the resolver is constructed inside
the store subscribe callback so renders discarded by concurrent
features cannot leak it. Each node's view subscribes only to that
node's props signal, so a data change re-renders exactly the affected
component.
Existing view code is reused unchanged: createComponentImplementation
now also exposes the unwrapped view, node props are converted back to
the string and {id, basePath} shapes views expect, and buildChild maps
those refs to live nodes. Refs the node layer cannot identify fall
back to DeferredChild, so the shipped basic catalog renders as-is.
Tests cover render isolation, template stability, StrictMode, action
dispatch, and input components writing scoped values through
synthesized setters.
Default rendering is unchanged; adding ?nodes to the URL renders every surface through A2uiNodeSurface so the two paths can be compared in the same app.
Every position the schema classifies as dynamic now resolves to a ResolvedBinding in node props: a snapshot value plus a set function present only when the payload bound a data path. The binder's synthesized set<Prop> siblings no longer appear in node props, so a write to a literal-valued property is a type error instead of a silent no-op. A2uiNodeSurface unwraps bindings back to the value + set<Prop> pair the existing views consume.
Remove wording that only made sense relative to the design discussions
("legacy", "binding contract", "payload spelling"), coined terms with no
in-code anchor ("emission gate", "prop-pair shape"), and comments that
restated adjacent assertions. Point change-detection docs at the concrete
mechanism, the shallow comparison in ComponentNode.setProps, and state
setProps's reference-identity requirement as a caller obligation rather
than a description of current callers.
9a83ab2 to
7115148
Compare
Removing the last waiting parent, or disposing the last waiting node, previously left an empty set behind, so the map grew with every component id that ever had a placeholder.
A component schema refined or transformed at the top level, or on a property, previously hid its child references from classification.
effect runs its body synchronously on creation, so the explicit call notified React twice for the same state.
Mutation moves to MutableComponentNode, which the package barrel does not export: application code reads props and writes data only through a binding's set. The base constructor is protected, so node construction is not public API either.
… them Per-usage .describe() on ComponentIdSchema and ChildListSchema replaced the whole description, silently dropping the REF: pointer the capabilities generator turns into wire $refs. Compose prose with the pointer via componentIdWithDescription and childListWithDescription instead, and sweep the basic catalog call sites. The node layer's ref-field classification now reads the surviving pointers directly, replacing its placeholder marker helpers, so the unmodified basic catalog resolves single-child references node-side.
view now takes {node, buildChild}; both factories generate it. The
generated wrapper subscribes to its node's props, converts them to the
ReactA2uiComponentProps shape existing views implement, and resolves
child ids through the conversion's index, work the surface previously
did once, untyped, for all components. A2uiNodeSurface shrinks to a
dispatcher that renders each implementation's view, with a
raw-definition fallback for implementations that lack one.
A view written directly against NodeViewProps needs a way to subscribe to node.props; only the generated wrappers could reach the hook.
…ntal # Conflicts: # renderers/web_core/src/v0_9/basic_catalog/components/basic_components.ts # renderers/web_core/src/v0_9/processing/message-processor.test.ts
2 participants
Implements #1282 for TypeScript,
Adds an experimental layer to
@a2ui/web_corethat resolves a surface's components and data into a live tree, plus a React surface that renders from that tree.Today,
A2uiSurfacerenders by recursing over component ids: each component wrapper runs its ownGenericBinderagainst raw definitions, child references stay strings until a wrapper renders them, and any renderer must reimplement the same resolution. The newNodeResolvermoves that work intoweb_core: it turns a surface's flat component map into a live tree ofComponentNodes, exposed through a reactiverootNodesignal. In a resolved node's props:ComponentNodeobjects (templateChildLists spawn one node per array item);ResolvedBinding: a snapshot plus asetfunction present only when the payload bound a data path, so writing to a literal-valued property is a type error instead of a silent no-op;A node's props signal emits only when that node's own resolved properties change; child-internal changes stay on the child's signal. Missing components and unknown catalog types render as placeholder nodes that are upgraded in place when resolvable; cyclic references render as placeholders with a dispatched
CYCLIC_REFERENCEerror. Node identity is parent-scoped, so one component id mounted in two positions yields two nodes and dropping one never tears down the other.A2uiNodeSurface(in@a2ui/react) is a drop-in sibling ofA2uiSurfacethat renders this tree. Each component subscribes only to its own node's props, so a data change re-renders exactly the affected component. Existing view code runs unchanged: a generated per-implementationviewconverts node props back to the value +set<Prop>and string-id shapes current views expect, and child references the catalog schema does not declare fall back to the existingDeferredChildrecursion, so third-party catalogs render as-is.Behavioral changes
.describe()calls onComponentIdSchema/ChildListSchemawere dropping theREF:pointer, so the basic catalog's eight child-reference properties (across Card, Row, Column, List, Tabs, Modal, and Button) were emitted as inline string schemas. They now emit$ref: common_types.json#/$defs/ComponentId(orChildList) as intended. This fix stands on its own, independent of the node layer, and addresses theREF:-handling part of [Proposal] Resolve child components based on schema and not on hardcoded 'child'/'children' keywords. Also improve REF: handling #1528 (the issue's larger ask, schema-driven child resolution in the existing binders, is untouched here).A2uiExpressionErrornaming the function as schema-only. On the rendering path this surfaces as the sameEXPRESSION_ERRORclient error as before, with a clear message where aTypeError's used to be.ComponentNode,NodeResolver,ResolvedBinding, and support types) from@a2ui/web_core/v0_9, andA2uiNodeSurfacewith its view types from@a2ui/react/v0_9. The twoindex.tsdiffs are the full list.A2uiNodeSurfacewhen?nodesis in the URL; default rendering is unchanged.viewrender through the fallback, andA2uiSurfaceis untouched.Known limitation: a template-spawned node's
instanceIdnames a list position, not a data item, so it is not stable across array insertions or reorders. Data-derived child keys depend on #1745.If this PR is adopted,
A2uiSurfacecan take these internals under its existing name andA2uiNodeSurfaceis deleted; the conversion innode-view.tsxexists only so current views render unchanged while the two are compared. Later, once catalog components are authored against resolved values directly (the portable web components proposal, #931, could be the place to do this), that conversion andGenericBinder's synthesizedset<Prop>shape lose their last consumer.