Fix children prop bug in the v0.9 Angular ComponentBinder and improve unit tests to use real models#1472
Conversation
…odels Refactored the ComponentBinder unit tests to use real, live instances of ComponentContext, ComponentModel, and SurfaceModel instead of fragile and verbose mocks. Additional extensive test cases have been added to cover all possible binding declarations specified in the A2UI v0.9 protocol: - Basic literal property bindings (strings, numbers, booleans, objects, null) - Reactive path-based property bindings (with verification against the actual DataModel) - Single child/trigger/content bindings with fallback handling for falsy values and existing Child objects - Extensive children list bindings covering static array of string IDs, resolved dynamic path array elements, pre-formatted Child objects, and template-based ChildList object expansions - Robust checks validation, testing dynamic reactivity and evaluation of multiple validation errors directly against the DataModel state This refactoring fixes a potential null-pointer TypeError bug when resolving children properties with null/falsy values. Improving these unit tests with real implementations provides a highly reliable safety net for future refactoring of the binder implementation while ensuring its behavior remains unchanged.
There was a problem hiding this comment.
Code Review
This pull request refactors the ComponentBinder test suite by introducing a helper function for context creation and significantly expanding test coverage for property binding, child resolution, and validation logic. In the service itself, optional chaining was added for children property access. Feedback includes a critical bug report regarding a variable leak in the property binding loop, a potential memory leak due to the service's lifecycle management, and a suggestion to return a promise in a test helper to prevent runtime errors.
e6ff9fb to
8bdf542
Compare
andrewkolos
left a comment
There was a problem hiding this comment.
The change looks good to me, and I verified that the new tests fail before the change 👍 . However, I'm withholding approval per ditman's comment. Feel free to re-request review when addressed
|
This PR fixes the null de-referencing TypeError when is null/undefined (tracked in #1477). |
|
Thanks for the review! Ready for another pass |
ditman
left a comment
There was a problem hiding this comment.
Thanks for finding and fixing this! Let's go!
|
Before you land this, please rename the PR to address what the bug is fixing, it's more than tests now! |
Description
This PR refactors the Angular
ComponentBinderunit tests for the v0.9 specification to use real, live instances ofComponentContext,ComponentModel, andSurfaceModelfrom@a2ui/web_core, completely eliminating fragile, verbose, and easily out-of-date mocks.It also addresses two critical bugs identified in the binder service implementation and adds robust validation coverage.
Motivation
The primary motivation behind this refactoring is to create a highly reliable, realistic safety net. This enables future refactoring of the
ComponentBinderimplementation with complete confidence, ensuring that its behavior remains completely unchanged and fully compliant with the v0.9 protocol specification.Key Changes & Added Test Cases
DataModelstate rather than call spies.DataModel.ChildListexpansions.Important Bug Fixes Included
templatevariable was declared outside the properties loop. When processing a component with achildrenlist that defined a template, that template would get leaked and incorrectly attached to all subsequent properties.templatedeclaration inside thefor (const key of Object.keys(props))loop so it is properly reset toundefinedfor each property.should not leak template to subsequent propertiesto prevent regression.TypeErrorduring componentId resolution inComponentBinder.bind.value?.componentIdandvalue?.path).should handle null/falsy or non-array values by returning an empty array.Fixes #1477