Implement a @nonreactive directive for selectively skipping reactive comparisons of query result subtrees
#10722
Conversation
This commit modifies the `isDifferentFromLastResult` logic in `ObservableQuery` so it skips over `@nonreactive` fields (thereby ignoring those subtrees of the query) whenever the `@nonreactive` directive is present in the query. When the `@nonreactive` directive is not present in the query, `isDifferentFromLastResult` continues using the `equal` function from `@wry/equality` to compare the results, which ensures the `@nonreactive` behavior is opt-in, preserving backwards compatibility. This functionality builds on the `compareResultsUsingQuery` predicate function introduced in PR #10237.
The hasRemoveDirective logic was broken because it did not enforce that the _matching_ directive config has a `config.remove` property. This led to the inappropriate removal of directives without `config.remove`, in situations when any other configs are present with `config.remove` set to `true`, even if those other configs do not match any directives found on the current AST node.
Faced with the prospect of performing a third whole-document traversal to remove the `@nonreactive` directive, in addition to the traversals already responsible for removing `@client` and `@connection`, I decided it was time to use the full power of `removeDirectivesFromDocument` to remove all three directives in one traversal.
These changes allow the latest equivalent result for queries using the `@nonreactive` directive to be stored in this.last.result, rather than being discarded in favor of an older (equivalent but different) result.
This comment was marked as resolved.
This comment was marked as resolved.
benjamn
changed the title
@nonreactive directive@nonreactive directive for selectively skipping reactive comparisons of query result subtrees
src/core/ObservableQuery.ts
Outdated
| const resultIsDifferent = | ||
| this.queryManager.transform(query).hasNonreactiveDirective | ||
| ? !compareResultsUsingQuery( | ||
| query, | ||
| this.last.result, | ||
| newResult, | ||
| this.variables, | ||
| ) | ||
| : !equal(this.last.result, newResult); |
There was a problem hiding this comment.
This is where we handle the backwards compatibility logic of using compareResultsUsingQuery only when the query contains one or more @nonreactive directives.
Despite appearances, this.queryManager.transform(query).hasNonreactiveDirective is cached after the first time query is encountered, so we do not have to keep scanning the query for @nonreactive directives every time this code runs.
| it.each<TypedDocumentNode<{ list: Item[] }>>([ | ||
| // This query uses a basic field-level @nonreactive directive. | ||
| gql` | ||
| query GetItems { | ||
| list { | ||
| id | ||
| text @nonreactive | ||
| } | ||
| } | ||
| `, |
There was a problem hiding this comment.
I love how easy it is to add variations of test cases using it.each!
| const serverQuery = removeDirectivesFromDocument([ | ||
| removeClientFields ? { name: 'client', remove: true } : {}, | ||
| { name: 'connection' }, | ||
| { name: 'nonreactive' }, | ||
| ], transformed); |
There was a problem hiding this comment.
Like @client and @connection, the @nonreactive directive is removed from queries before they are sent to the server, though (unlike @client) only the @nonreactive directive is removed, preserving the field/fragment it was annotating (because we want all the data from the server, even if the client chooses not to broadcast it).
Thanks to 3030094, all three of these removals can be performed in the same traversal of the AST!
|
/release:pr |
|
A new release has been made for this PR. You can install it with |
These files seem to be following the rules already, but this will help enforce the style going forward. #10722 (comment)
There was a problem hiding this comment.
Alright @jerel and @alessbell, I think I've addressed all the feedback except for the "testing-library/no-wait-for-multiple-assertions": "off" change. Please take another look!
benjamn
added
💡 idea
🧞♂️ enhancement
🧪 has-tests
🥚 backwards-compatible
As foretold in #8694 (comment), and building on work started in #10237, this PR adds support for a new query directive called
@nonreactive, which can be used to mark query fields or fragment spreads, indicating changes to the data contained within the subtrees marked@nonreactiveshould not trigger rerendering, allowing parent components to fetch child data without rerendering themselves every time something changes about one/some of the children (assuming the child data is marked@nonreactivein the parent query).This selective insensitivity of parent components to certain child data was originally planned to be a feature of
useBackgroundQuery, but that hook has turned into something else, and@nonreactiveseems like a better solution for the original problem anyway.Since
@nonreactiveis implemented at a point in the query pipeline where we were already performing a deep equality check (isDifferentFromLastResultinObservableQuery), and since we "compare"@nonreactivesubtrees by ignoring them completely (cheap!), the use of@nonreactivehas the potential to allow much faster comparisons.However, because this behavior is slightly different from the previous deep equality check (for example, the previous implementation was sensitive to extraneous fields in the result not mentioned by the query), the new behavior (using
compareResultsUsingQuery) is only enabled when the query contains at least one@nonreactivedirective. Since no one is using the@nonreactivedirective yet, all existing result comparisons should continue to work exactly as before.I recommend reviewing this PR commit by commit, and reading the commit messages, since they explain several bug fixes and refactorings to support the
@nonreactiveimplementation.