fix(a2ui_core): distinguish between "key": null and absence of key in DataModel#938
fix(a2ui_core): distinguish between "key": null and absence of key in DataModel#938andrewkolos wants to merge 5 commits into
"key": null and absence of key in DataModel#938Conversation
…taModel
The v0.9 spec treats "value: null" and an omitted value key as different
operations on updateDataModel:
value: null -> set the key to null
value absent -> remove the key (for list indices, set the slot to null
and preserve length)
a2ui_core collapsed both into "remove the key" at two layers:
1. The message parser dropped the wire distinction into a single
Object? value field, and toJson dropped null values on serialize.
Round-tripping a "value: null" message through fromJson + toJson
lost the key.
2. DataModel.set(path, null) removed the key. There was no public
API to set a key to literal null.
web_core gets it right end-to-end. Its MessageProcessor preserves
the JS undefined-vs-null distinction; its DataModel.set deletes on
undefined and assigns on null.
Changes:
- UpdateDataModelMessage gets a hasValue field and a .removeKey
named constructor. fromJson picks between them based on whether
the 'value' key is present.
- DataModel.set(path, null) now sets to null. New DataModel.remove
does the deletion. For map keys it deletes the key; for list
indices it sets the slot to null and preserves length. No-op on
missing paths. remove('/') resets the data model to an empty map.
- MessageProcessor dispatches: hasValue ? set : remove.
Breaking change for a2ui_core consumers that relied on set(path, null)
meaning "remove the key". Switch those calls to remove(path).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Package publishing
Documentation at https://github.com/dart-lang/ecosystem/wiki/Publishing-automation. |
|
/gemini review |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the DataModel to distinguish between setting a key to an explicit null value and removing a key entirely. It introduces a new remove method in DataModel, updates UpdateDataModelMessage with a removeKey constructor and a hasValue flag to support this distinction during serialization, and updates the MessageProcessor to call either set or remove accordingly. Comprehensive unit tests have been added to verify these changes. I have no review comments or feedback to provide on these changes.
| } | ||
|
|
||
| /// Removes the value at [path]. For map keys this deletes the key; for | ||
| /// list indices it sets the slot to null and preserves length. |
There was a problem hiding this comment.
When are the nulls in lists cleaned up? And how are they handled when rendering? Wouldn't you expect the list to change size if you remove an entry?
There was a problem hiding this comment.
Great questions that I didn't think much about since I was really only thinking about conforming to the current a2ui renderer guide (as best could be done).
When are the nulls in lists cleaned up?
They aren't. For all intents and purposes, the server would need to replace the list with a new, compacted list. This PR treats list removal as preserving length and setting the slot to null (data_model.dart#L62-L81).
How are they handled when rendering?
They are still iterated over. genui does not filter null entries during list-template iteration: ComponentChildrenBuilder delegates template-list rendering to the catalog widget (widget_helpers.dart), and List builds one child per list index (list.dart#L99-L124). a2ui_core does the same thing: the binder generates a ChildNode for each list index with the corresponding scoped base path (binder.dart#L124-L133).
This is also what the v0.9 renderer guide specifies. It says setting an array index to undefined preserves length and empties the index (renderer_guide.md#L284). Dart/JSON do not have JS-style undefined, and JSON cannot represent sparse array slots. web_core can keep undefined in-memory. In a2ui_core, the equivalent emptied array slot is represented as null (I suppose we could use maps and represent empty list slots that way...but yeah that's intuitive complexity for behavior I am not even 100% sure is worth preserving).
This whole situation is a bit complex and not ideal, but I'll get to that in my next point.
Wouldn't you expect the list to change size if you remove an entry?
Intuitively, yes, but that's not under the current v0.9 semantics. “Remove object key” and “empty array slot” are different operations. If we want remove-from-list to mean splice/compact the list, I think that should be a spec/API change rather than part of this PR.
I can file an issue over in the A2UI repo (as well as one here) to track work to make the answers to your questions intuitive. WDYT?
- _onCoreSurfaceCreated: migrate fallback data via store.attachLive BEFORE registry.addSurface notifies listeners, so a synchronous ValueListener callback that calls contextFor.dataModel sees the populated live model rather than racing the migration. - DataModelStore.getDataModel: check the _liveDataModels cache before invoking the lookup callback. Avoids redundant wrap calls and avoids the race where a getDataModel call cached one wrapper just before attachLive cached another. - DataModelStore.attachLive: drop the null-snapshot guard so a pre-create `update(root, null)` is preserved across createSurface. Adds a test. - Revert the genui.dart hide of SurfaceRegistry. SurfaceRegistry existed as a public type pre-migration; hiding it would be an unrelated source break. The new core-typed `surface` fields on SurfaceAdded / SurfaceUpdated remain `@internal`. - Migration guide: add an explicit Scope section listing the deferred follow-ups (catalog widget bodies, action dispatch, sendDataModel, GenericBinder, typed-props), the flutter#938 dependency for the hasValue/remove runtime split, and the A2UI#1499 RFC 6901 tracking.
2 participants
See #935.
This bug is also present in
genui. However, to reduce churn, I recommend just waiting until we rebuildgenuion top ofa2ui_core.