-
Notifications
You must be signed in to change notification settings - Fork 97
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
Fix ko.observable handling in Linq translations #1870
base: main
Are you sure you want to change the base?
Conversation
exyi
commented
Oct 16, 2024
- FirstOrDefault, LastOrDefault, ... should behave like indexers
- return observable, if the array contains observables, which then needs to be unwrapped
- this is a regression in 4.3
- Where, Order, Take, ... return the same structure which the original array has
- Select is weird as it unwraps all observables in the array, but not recursively. That's why patch adds the JsObjectObservableMap object, which can represent this mid-state (and many other combinations)
- Indexing after Select was apparently always broken in DotVVM
This is a lighterweight alternative to #1870, which we might not want to merge to 4.3. This PR does only fixes the 4.3 regression, the problem with .Select(...).ToArray()[0] is not fixed
* FirstOrDefault, LastOrDefault, ... should behave like indexers - return observable, if the array contains observables, which then needs to be unwrapped - this is a regression in 4.3 * Where, Order, Take, ... return the same structure which the original array has * Select is weird as it unwraps all observables in the array, but not recursively. That's why patch adds the JsObjectObservableMap object, which can represent this mid-state (and many other combinations) - Indexing after Select was apparently always broken in DotVVM
bf2e517
to
43e5a18
Compare
43e5a18
to
562c26f
Compare
|
||
public Dictionary<string, bool>? PropertyIsObservable { get; set; } | ||
|
||
public bool? IsPropertyObservable(string? name) => name is {} && PropertyIsObservable?.TryGetValue(name, out var value) == true ? value : ContainsObservables; |
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.
Add doc comment about meaning of null
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.
Please move to a separate PR.
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.
Move this and tests-related change to a separate branch so we can cherry-pick it to 4.3 also.