Add a framework-agnostic data layer to the web core library#631
Add a framework-agnostic data layer to the web core library#631jacobsimionato wants to merge 12 commits intogoogle:mainfrom
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
The pull request introduces a framework-agnostic data layer for the web core library, aligning it with the v0.9 specification and the Flutter implementation. It features a reactive DataModel with JSON Pointer support, a MessageProcessor for protocol handling, and Zod-based schema validation for components. The architecture is well-decoupled, allowing for easier integration with different web frameworks. However, the relaxation of TypeScript strictness in tsconfig.json is a concern, and there are potential edge cases in the data model's path resolution and array handling that should be addressed to ensure robustness.
|
|
||
| /* Linting */ | ||
| "strict": true, | ||
| "strict": false, |
There was a problem hiding this comment.
Disabling strict mode in a core library is a significant regression in type safety. It is highly recommended to keep strict: true to catch potential null/undefined errors and ensure better code quality, especially for a library intended to be used across different frameworks.
| "strict": false, | |
| "strict": true, |
| if (current[segment] === undefined || current[segment] === null) { | ||
| const nextSegment = (i < segments.length - 1) ? segments[i + 1] : lastSegment; | ||
| current[segment] = /^\d+$/.test(nextSegment) ? [] : {}; | ||
| } | ||
| current = current[segment]; | ||
| } |
There was a problem hiding this comment.
The set method does not handle cases where an intermediate path segment exists but is a primitive value (e.g., a string or number). If the path traversal encounters a primitive, subsequent property access or assignment will fail or behave unexpectedly. It would be safer to check if current[segment] is an object before proceeding or to overwrite it if the path implies a nested structure.
|
|
||
| if (value === undefined) { | ||
| if (Array.isArray(current)) { | ||
| current[parseInt(lastSegment, 10)] = undefined; |
There was a problem hiding this comment.
Setting an array element to undefined using an index assignment creates a "hole" in the array and does not change its length. If the intention is to remove the element from the array (consistent with delete for objects), Array.prototype.splice() should be used instead.
| current[parseInt(lastSegment, 10)] = undefined; | |
| current.splice(parseInt(lastSegment, 10), 1); |
| } | ||
| } else if (msg.updateDataModel) { | ||
| const payload = msg.updateDataModel; | ||
| const path = payload.path || '/'; |
There was a problem hiding this comment.
The routeMessage method uses if/else if blocks to process updateComponents and updateDataModel. This means that if a single message object contains both keys, only the first one encountered will be processed. While the protocol might currently favor one command per message, it's more robust to process all present commands by using separate if blocks.
| const path = payload.path || '/'; | |
| } | |
| if (msg.updateDataModel) { |
| } | ||
|
|
||
| private parsePath(path: string): string[] { | ||
| return path.split('/').filter(p => p.length > 0); |
There was a problem hiding this comment.
The parsePath method uses a simple split on /, which does not support standard JSON Pointer escaping (e.g., ~1 for / and ~0 for ~). If the data model is intended to be fully compliant with JSON Pointer (RFC 6901), these escape sequences should be handled during path parsing.
| if (component && component !== existing.type) { | ||
| console.warn(`Attempting to change type of component ${id} from ${existing.type} to ${component}. Ignoring new type.`); | ||
| } |
There was a problem hiding this comment.
When a component already exists, the code warns and ignores a type change. However, if the server intentionally changed the component type for a specific ID, simply updating the properties of the old type might lead to an invalid state or rendering errors. A more robust approach would be to recreate the component if the type discriminator changes.
Renames the 'A2uiModel' class to 'SurfaceGroupModel' and its corresponding files to better reflect its role in managing groups of surfaces. Updates all internal references and tests accordingly.
…d ComponentsModel Refactors SurfaceGroupModel and ComponentsModel to accept instances via `addSurface` and `addComponent` respectively, improving encapsulation. Implements an observable pattern for action handling in SurfaceModel and SurfaceGroupModel, removing the ActionHandler dependency from the SurfaceModel constructor.
Renames ComponentsModel to SurfaceComponentsModel to better reflect its scope and relationship with SurfaceModel. Updates all references and exports.
1 participant
No description provided.