feat: Add hasReactiveAsyncProperty to fix reactivity on properties. #619
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Currently, reactive rules and reactive fields are allowed to depend on async properties.
There are tests for this behavior that verify the reactivity, however in retrospect they only covered reactivity due to gross mutations to the object graph, i.e. only fields that were present in the load hint.
For example, given an async property such as:
Any reactive rules/fields that depended on this
isPublic2
would be recalculated if theBookReview.comment
FK field ever changed, becausecomment
existed in the load hint which Joist internally used asisPublic2
's reactive hint.However, any non-graph-mutating changes, such as simple changes to primitive fields that were used within the
hasAsyncProperty
lambda, would not trigger reactivity.There are few different approaches to fix this:
Keep the "reactives rules/fields can use async properties w/just load hints" but embellish the load hint with every primitive the lamba could possibly read.
I.e. a load hint like
{ book: "review" }
would be embellished to beEven if the lambda only accessed
rating
, any mutation toBookReview
(even ifrating
is unchanged) would trigger potentially unnecessary reactivity/recalcs of dependent reactive rules/fields.Break any current reactive rules/fields that use
hasAsyncProperty
s, and force them to instead write the property with a newhasReactiveAsyncProperty
.The new
hasReactiveAsyncProperty
is semantically exactly the same ashasAsyncProperty
, except that it takes a reactive hint, and hence it's lambda is forced to specify which fields it actually will access.Given that
hasReactiveAsyncProperty
has a user-provided reactive hint, there is no need for "embellishment", and we can hook up reactivity now with assurances that even primitive field changes will trigger the appropriate reactivity.Require all
hasAsyncProperty
s to use a reactive hint instead of a load hint.This PR implements the 2nd option.