Skip to content

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

rbalicki2
Copy link

  • I took the liberty of reading and adding some additional details to the pitfalls-of-normalization doc. This is a great doc!

@rbalicki2
Copy link
Author

@microsoft-github-policy-service agree

@rbalicki2
Copy link
Author

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.

Copy link
Contributor

@vladar vladar left a 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.

Comment on lines +173 to +177
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.
Copy link
Contributor

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).

Comment on lines +596 to +597
- 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.
Copy link
Contributor

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)

Comment on lines +600 to +602
- 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.
Copy link
Contributor

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants