-
Notifications
You must be signed in to change notification settings - Fork 733
Refactor the Lit renderer to introduce a concept of CatalogApi and CatalogImplementation etc #487
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a significant architectural refactoring to the A2UI Lit renderer, aligning it with a new framework-agnostic type system design. Key changes include the introduction of ComponentApi definitions for each standard component (e.g., AudioPlayer, Button, Card), a CatalogApi to manage these definitions, and a FrameworkRenderer (LitRenderer) that uses a CatalogImplementation to map component APIs to their Lit-specific ComponentRenderer implementations. The A2uiMessageProcessor has been updated to leverage this new catalog-based resolution, replacing a large switch statement with a more extensible approach. Individual Lit components (a2ui-text, a2ui-card, etc.) have been updated to receive a generic node property and a renderChild callback, decoupling them from direct knowledge of the message processor and enabling recursive rendering. The Root base class for Lit components now uses Lit Context to provide MessageProcessor and SurfaceID to its descendants, addressing a previous issue where leaf components couldn't resolve data bindings. Debugging logs have been added throughout the A2uiMessageProcessor and Lit components, and the package.json exports and internal import paths have been updated to reflect the new file structure. Review comments highlight the need to remove temporary console logs, improve encapsulation by adding a public method to CatalogApi for iterating components, and make the child property in CardNode optional to match its usage.
| } | ||
|
|
||
| private setDataByPath(root: DataMap, path: string, value: DataValue): void { | ||
| console.log(`[setDataByPath] Path: ${path}, Value:`, value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file and several others in the PR (e.g., surface.ts, text.ts, lit_renderer.ts) include many console.log and console.warn statements. While these are helpful for debugging during development, they should be removed before merging to avoid cluttering the browser console in production. Please consider removing all temporary debugging logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While this is true, I think we should consider add some logging capabilities to the library through some Logger interface. I don't know if this needs to be homebrew, or we can pull in some dependency to have it, but it'd be useful to have an entrypoint on the library to inject some observability into its innards :)
| for (const api of (catalogApi as any)['components'].values()) { | ||
| if (!this.renderers.has(api.name)) { | ||
| throw new Error(`Missing renderer implementation for component: ${api.name}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Accessing the private components property of CatalogApi using (catalogApi as any)['components'] is not ideal as it breaks encapsulation and is brittle. A better approach would be to expose a public method on CatalogApi to iterate over its components.
For example, you could add the following method to the CatalogApi class:
// In CatalogApi class
public getApis(): IterableIterator<AnyComponentApi> {
return this.components.values();
}Then you can use it here.
| for (const api of (catalogApi as any)['components'].values()) { | |
| if (!this.renderers.has(api.name)) { | |
| throw new Error(`Missing renderer implementation for component: ${api.name}`); | |
| } | |
| } | |
| for (const api of catalogApi.getApis()) { | |
| if (!this.renderers.has(api.name)) { | |
| throw new Error(`Missing renderer implementation for component: ${api.name}`); | |
| } | |
| } |
| child: AnyResolvedNode; | ||
| children: AnyResolvedNode[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The child property in CardNode is currently typed as required (AnyResolvedNode), but the resolveProperties function and the component's render function can handle it being undefined (for example, when only children is provided). This can lead to type mismatches. It would be better to make child optional.
| child: AnyResolvedNode; | |
| children: AnyResolvedNode[]; | |
| child?: AnyResolvedNode; | |
| children: AnyResolvedNode[]; |
| throw new Error('Invalid properties for Card: missing child or children.'); | ||
| } | ||
|
|
||
| const child = resolver(unresolved.child) as AnyResolvedNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type assertion as AnyResolvedNode is unsafe here. If unresolved.child is undefined, resolver(unresolved.child) will return undefined, and the cast will hide a potential type error. It's better to handle the optional nature of child explicitly, especially after making it optional in the CardNode interface.
| const child = resolver(unresolved.child) as AnyResolvedNode; | |
| const child = unresolved.child ? resolver(unresolved.child) as AnyResolvedNode : undefined; |
types_design.md
Outdated
| * When the signal changes, it passes the new resolved node tree to the `frameworkRenderer.renderNode()`. | ||
| * The `FrameworkRenderer` recursively walks the tree, using the `CatalogImplementation` to find the right `ComponentRenderer` for each node, until the entire tree is converted into the framework's renderable format. | ||
|
|
||
|  |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| export interface DateTimeInputNode extends BaseResolvedNode<'DateTimeInput'> { | ||
| properties: { | ||
| value: StringValue; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some implementation, value is reserved for input[type=datetime]. To avoid conflict, use a different property name like dtValue, see https://source.corp.google.com/piper///depot/google3/third_party/a2ui/renderers/lit_internal/src/v0_8/ui/datetime-input.ts;l=28#:~:text=27-,28,-29
| `, | ||
| ]; | ||
|
|
||
| #renderAudio() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Google3 does not like #, use private renderAudio instead.
| </section>`; | ||
| } | ||
|
|
||
| #getInputType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace # with private .
| return value.toString().padStart(2, "0"); | ||
| } | ||
|
|
||
| #getPlaceholderText() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace # with private
| return this.#weight; | ||
| } | ||
|
|
||
| #weight: string | number = 1; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace # with private
|
|
||
| #renderLogo() { | ||
| if (!this.surface?.styles.logoUrl) { | ||
| #renderLogo(surface: SurfaceState) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replace # with private .
Summary
This PR implements a major architectural refactor of the A2UI web renderers, aligning the Lit implementation with the design principles outlined in
types_design.md. The primary goal is to decouple the core A2UI message processing logic from framework-specific rendering details, creating a robust, type-safe, and extensible foundation for the A2UI ecosystem.Architectural Changes (Design Focus)
1. Framework-Agnostic Core
The core logic has been extracted and separated from Lit-specific code:
A2uiMessageProcessor: Now a standalone, framework-agnostic engine residing incore/a2ui_message_processor.ts. It handles message processing, state management, and node resolution without any knowledge of the rendering layer.ComponentApi&CatalogApi: Introduced purely abstract definitions for components and catalogs incore/types. This allows defining a component's contract (e.g., properties, resolution logic) independent of its visual implementation.2. Rendering Abstraction
We introduced new interfaces to bridge the core logic with specific frameworks:
ComponentRenderer: Defines the interface for converting a fully resolved node into a specific framework's output (e.g.,TemplateResultfor Lit).CatalogImplementation: Maps aCatalogApito a set ofComponentRenderers. This layer ensures compile-time safety by enforcing that every component defined in the API has a corresponding renderer implementation.3. Directory Restructure
The codebase has been reorganized to reflect this separation of concerns:
src/0.8/core/: Contains all framework-agnostic code, including types, guards, the message processor, and standard catalog definitions.src/0.8/lit/: Contains Lit-specific implementations, including theLitRenderer, component renderers, and web components.Key Improvements & Fixes
Surfacecomponents would fail to render if a renderer wasn't explicitly provided. TheSurfaceclass now correctly initializes with a defaultstandardLitCatalogImplementation.A2uiMessageProcessorconstructor now accepts aCatalogApi, enabling dynamic injection of component definitions.AnyResolvedNode,CatalogApi) to fully support the new architecture.package.jsonexports to point to the correct paths for the new core and UI modules.Debugging Notes
renderers/lit/fixing.mdto document the investigation into rendering issues (specifically the "no model" issue) and the solutions applied during this refactor.Next Steps
ProcessorContext(using Lit Context) to resolve the pending issue where data-bound components render(no model). This will provide descendant components access to theMessageProcessorinjected at theSurfacelevel.