Remove FragmentMatcher abstraction from apollo-cache-inmemory. #5073
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
benjamn
force-pushed
the
remove-fragment-matcher-abstraction
branch
from
56d14ec
to
450107a
Compare
benjamn
commented
Jul 18, 2019
| ); | ||
| } | ||
| } else if ( | ||
| context.possibleTypes && |
There was a problem hiding this comment.
We have a number of tests that verify warnings about missing fields, but we can’t be completely confident those fields are missing unless we have accurate possibleTypes information. This is especially important if we ever want to pursue the comment below (re: throwing instead of warning).
benjamn
commented
Jul 18, 2019
| const workQueue = [possibleTypes[typeCondition]]; | ||
| // It's important to keep evaluating workQueue.length each time through the | ||
| // loop, because the queue can grow while we're iterating over it. | ||
| for (let i = 0; i < workQueue.length; ++i) { |
There was a problem hiding this comment.
Cycle-safe breadth-first search without using recursion! 🤓🔂🦄
|
@hwillson The |
benjamn
commented
Jul 18, 2019
| @@ -1218,41 +1182,16 @@ describe('client', () => { | |||
| ], | |||
| }; | |||
|
|
|||
| const fancyFragmentMatcher = ( | |||
There was a problem hiding this comment.
Scroll down to find out what I was able to replace this fancy fraggle doodad with…
benjamn
commented
Jul 18, 2019
| cache: new InMemoryCache({ | ||
| possibleTypes: { | ||
| Item: ['ColorItem', 'MonochromeItem'], | ||
| }, |
There was a problem hiding this comment.
It’s just so much shorter!
hwillson
approved these changes
Jul 19, 2019
There was a problem hiding this comment.
This is great @benjamn - simpler is better! 🙂 The only comment I really have is around whether possibleTypes is a bit too generic as a constructor param name, but thinking it over a few times it does seem to be the best name. So, disregard - looks great!
On a related note - should we kickstart a 3.0 branch?
Correctly matching fragments against abstract types like unions or
interfaces requires knowledge of the schema, which can be obtained in
multiple ways.
Previously, we provided two different FragmentMatcher implementations: the
HeuristicFragmentMatcher, which would attempt to match fragments without
any knowledge of the schema; and the IntrospectionFragmentMatcher, which
would consume an introspection query result to obtain information about
possible subtypes.
This commit removes the FragmentMatcher abstraction and both of its
implementations. Instead, you can skip the introspection query step and
simply pass a possibleTypes map to the InMemoryCache constructor:
new InMemoryCache({
addTypename: true,
freezeResults: true,
possibleTypes: {
Animal: ["Cat", "Dog", "MiniatureHorse", "Frog"],
Test: ["PassingTest", "FailingTest", "SkippedTest"],
},
})
When you provide this information, assuming it is correct and complete,
the cache does not need to make any heuristic guesses, so there's really
no point in ever having other FragmentMatcher implementations.
More importantly, to maintain backwards compability with the existing
FragmentMatcher interface, the cache was forced to construct temporary
data stores to trick the FragmentMatcher into doing the right thing. This
commit is a breaking change that removes all that awkward logic.
Of course, you have to get the possibleTypes map from somewhere, and
schema introspection is still a fine way of doing that. However, the
possibleTypes map is relatively easy to write by hand, if you're just
prototyping/experimenting. Once you're ready to generate it automatically
from your schema, it can be easily imported from a generated module.
If the possibleTypes option is not specified, the cache will behave like
the old HeuristicFragmentMatcher, which is adequate as long as you're not
using fragments with abstract constraints.
In principle, the subtypes found in the possibleTypes map could have subtypes of their own! Instead of ignoring this possibility, we should embrace it fully and efficiently. Subtype indirection is not necessarily a mistake, since defining an abstract type once and then referring to it by name in multiple other places tends to be more space-efficient than inlining all the concrete subtypes into every possibleTypes array.
This avoids calling subtypes.indexOf in favor of an object key lookup. It's also now possible to call cache.addPossibleTypes after constructing the InMemoryCache, and the new types will be merged appropriately.
benjamn
force-pushed
the
remove-fragment-matcher-abstraction
branch
from
5f4eef3
to
9ccd310
Compare
benjamn
added a commit
that referenced
this pull request
Dec 13, 2019
Although we removed the FragmentMatcher abstraction in PR #5073, and the HeuristicFragmentMatcher no longer exists, we kept trying to match fragments whenever all their fields matched, even if the fragment type condition was not satisfied by the __typename of the object in question. I recently came to appreciate just how harmful this best-effort matching can be, when I realized that it means we call executeSelectionSet (on the reading side) and processSelectionSet (on the writing side) for every unmatched fragment, and then just throw away the result if any fields are missing, but not before recording those fields as missing in a weaker way than usual, using the ExecResultMissingField.tolerable boolean. Not only is this strategy potentially costly for queries with lots of fragments that really should not match, it also only ever made sense on the writing side, where the fields of the fragment can be compared to the fields of an incoming result object, so the presence of all the fragment's fields is a pretty good indication that the fragment matched. On the reading side, we're comparing the fields of the fragment to all the fields we've written into the cache so far, which means heuristic fragment matching is sensitive to the whole history of cache writes for the entity in question, not just the current query. We could eliminate heuristic fragment matching just for reading, but then we'd have to tell people to pass possibleTypes to the InMemoryCache constructor (the correct, non-heuristic solution), which would make heuristic fragment matching unnecessary on the writing side, too, so we might as well completely eliminate heuristic matching altogether.
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Correctly matching fragments against abstract types like unions or interfaces requires knowledge of the schema, which can be obtained in multiple ways.
Previously, we provided two different
FragmentMatcherimplementations: theHeuristicFragmentMatcher, which would attempt to match fragments without any knowledge of the schema; and theIntrospectionFragmentMatcher, which would consume an introspection query result to obtain information about possible subtypes.This commit removes the
FragmentMatcherabstraction and both of its implementations. Instead, you can skip the introspection query step and simply pass apossibleTypesmap to theInMemoryCacheconstructor:When you provide this information, assuming it is correct and complete, the cache does not need to make any heuristic guesses, so there's really no point in ever having other
FragmentMatcherimplementations.More importantly, to maintain backwards compatibility with the existing
FragmentMatcherinterface, the cache was forced to construct temporary data stores to trick theFragmentMatcherinto doing the right thing. This commit is a breaking change that removes all that awkward logic.Of course, you have to get the
possibleTypesmap from somewhere, and schema introspection is still a fine way of doing that. However, thepossibleTypesmap is relatively easy to write by hand, if you're just prototyping/experimenting. Once you're ready to generate it automatically from your schema, it can be easily imported from a generated module.If the
possibleTypesoption is not specified, the cache will behave like the oldHeuristicFragmentMatcher, which is adequate as long as you're not using fragments with abstract constraints. You only live once, as they say.