-
Notifications
You must be signed in to change notification settings - Fork 39
Add some additional details to the pitfalls doc #509
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
base: main
Are you sure you want to change the base?
Conversation
rbalicki2
commented
Feb 1, 2025
- I took the liberty of reading and adding some additional details to the pitfalls-of-normalization doc. This is a great doc!
@microsoft-github-policy-service agree |
cc @vladar. I can't add you as a reviewer, but you are probably the best person to review. Anyway, cheers, feel free to close if this isn't to your liking! Just was reading through the doc and made a few light additions as I went. |
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.
Hi! Thanks for your interest, I posted some comments inline.
However, note that _even if you cannot avoid a network request_, a normalized cache that integrates | ||
with suspense (such as Relay's) makes it very trivial to achieve the following behavior: | ||
when a user navigates from a list view to a detail view, the transition can occur instantaneously, | ||
and the components for which there is enough data (such as the detail view header) can render | ||
instantly. The rest of the detail view is replaced by a suspense boundary. |
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.
I feel this section doesn't belong here - it should probably go under Example 1. Missing fields
.
We should also clarify that it is only possible with UI libraries like React, supporting this kind of rendering interruption (not sure how it will work with something like Vue or Angular).
Also, I'd argue that the key requirement for this behavior is "fragment-level" binding between components and data, not normalization per se. As long as cache can return data for an individual fragment - this behavior will work (normalization is one possible implementation, but not the only one).
- Relatedly, no cross-query consistency, meaning that if a user modifies data on a given page and navigates back to a previous | ||
page, the data on the previous page will be inconsistent. Adds a burden on developers to invalidate in response to mutations. |
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.
I think this note misses the point of this experimental cache. See the beginning of this section:
query results are stored separately and are kept in sync on write without intermediate normalized store
Our experimental document-based cache keeps all operations up-to-date on incoming operations automatically. We just don't use full data normalization/re-reads for this, but instead use normalized diffs for incoming data, which are applied to all cached operations (currently - eagerly, but could be also applied lazily on reads).
So this example is not quite correct. Admittedly, the implementation turned out to be even more complicated in some aspects (incremental and normalized diffing / updating), but simpler in other (e.g. garbage collection is still rather trivial)
- One cannot obviously get [data masking][14] | ||
without introducing a step in which data is read from a source, so the simplicity gains may not be as drastic as assumed. | ||
- Inability to reuse data, meaning that transitions from list views to detailed views cannot be perceptually instant. |
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.
Depending on strictness of data-masking definition I guess. Type-level data-masking is possible today with any client via type-generation (e.g. with graphql codegen).
Full runtime data-masking requires additional filtering of data indeed, but I wonder how much value Exact<FragmentType>
vs FragmentType
actually provides, as long as types guard us against fields mis-use. A big one is probably higher memoization hit rate for React.memo
, but for performance-critical parts one could rely on fragment aliases to isolate frequently changing data (sadly not a part of the spec yet).
I also don't see conceptual blockers for this example with document-based cache:
transitions from list views to detailed views cannot be perceptually instant
(in our case this requires either using the same fragment in list/item view or explicitly annotating which queries could be used a source for a specific item fragment as a fallback)
But I completely agree with this statement:
so the simplicity gains may not be as drastic as assumed
Our experience shows that document-based cache with full syncing and fragment-level component binding is not much simpler than normalized cache in terms of implementation (most of the complexity in the existing implementation comes from a requirement to be a drop-in-replacement for Apollo InMemoryCache).
Co-authored-by: Vladimir Razuvaev <vladimir.razuvaev@gmail.com>