Skip to content
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

feat: Add hasReactiveAsyncProperty to fix reactivity on properties. #619

Merged
merged 2 commits into from
Apr 17, 2023

Conversation

stephenh
Copy link
Collaborator

@stephenh stephenh commented Apr 17, 2023

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:

class BookReview {
  readonly isPublic2 = hasAsyncProperty("comment", br =>
    return !br.comment.get?.text.includes("Ignore");
  });
}

Any reactive rules/fields that depended on this isPublic2 would be recalculated if the BookReview.comment FK field ever changed, because comment existed in the load hint which Joist internally used as isPublic2'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:

  1. 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 be

    { book: { "title": {}, review: { "rating", "otherFieldA", "otherFieldB" } }
    

    Even if the lambda only accessed rating, any mutation to BookReview (even if rating is unchanged) would trigger potentially unnecessary reactivity/recalcs of dependent reactive rules/fields.

  2. Break any current reactive rules/fields that use hasAsyncPropertys, and force them to instead write the property with a new hasReactiveAsyncProperty.

    The new hasReactiveAsyncProperty is semantically exactly the same as hasAsyncProperty, 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.

  3. Require all hasAsyncPropertys to use a reactive hint instead of a load hint.

This PR implements the 2nd option.

Previously, reactive rules and reactive fields were allowed to depend
on async properties.

There were tests against this behavior that seemed reactive, in that
they showed reactivity to gross mutations to the object graph, i.e.
_anything that was presented in the load hint_.

For example, given an async property such as:

class BookReview {
  readonly isPublic2 = hasAsyncProperty("comment", br =>
    return !br.comment.get?.text.includes("Ignore");
  });
}

Any reactive rules/fields that depended on this `isPublic2` would be
recalculated if the `BookReview.comment` field ever changed. Because
we passed the load hint along as a 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 were two potential approaches to fix this:

1. Keep the "reactives rules/fields can use async properties w/just
load hints" but embellish the load hint with the primitive of every
possible field the lamba could read.

I.e. a load hint like `{ book: "review" }` would be embellished to be

```
{ book: { "title": {}, review: { "rating", "otherFieldA", "otherFieldB" } }
```

Even if the lambda only accessed `rating`.

2. Instead of "embellishing load hints into reactive hints", break any
usage of a reactive rule/field on a load-hint-using hasAsyncProperty,
and force the user to rewrite the property with a new
hasReactiveAsyncProperty.

The new hasReactiveAsyncProperty is semantically exactly the same as
hasAsyncProperty, 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.

This PR implements the 2nd option.
Copy link
Collaborator

@zgavin zgavin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

our poor tortured toy data model 😂

@stephenh
Copy link
Collaborator Author

our poor tortured toy data model

Lol, yes; this was like my 3rd attempt at "how do I least painfully slice this in as new fields/behavior" :-)

@stephenh stephenh merged commit 350ef63 into main Apr 17, 2023
@stephenh stephenh deleted the fix-async-property-reactivity branch April 17, 2023 15:42
stephenh pushed a commit that referenced this pull request Apr 17, 2023
# [1.74.0](v1.73.2...v1.74.0) (2023-04-17)

### Features

* Add hasReactiveAsyncProperty to fix reactivity on properties. ([#619](#619)) ([350ef63](350ef63))
@stephenh
Copy link
Collaborator Author

🎉 This PR is included in version 1.74.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants