Allow registering named fragments with `InMemoryCache` to support using `...FragmentName` in queries without redeclaring `FragmentName` in every query #9764

benjamn · 2022-05-26T19:48:11Z

When a query uses a named fragment, or when fragments use other fragments by name, our current recommendation is to ${...}-interpolate the necessary fragment declarations into the query's gql template string.

While this pattern works in small examples, it gets cumbersome when there are multiple (layers of) fragments, and it's all too easy to end up duplicating fragment declarations in the resulting query, if they accidentally get interpolated twice (or more!).

This PR attempts to make this situation substantially easier, by allowing you to pre-register certain named fragments with your InMemoryCache, so that you can refer to them by name elsewhere, without needing to interpolate their declarations. That interpolation now happens behind the scenes, each time the query is about to be sent over the network. The dynamic nature of the interpolation guarantees each fragment will be included only once, only if/when it's used.

Queries can still declare their own local versions of named fragments, which will take precedence over the pre-registered ones, even if the local fragment is only indirectly referenced by other fragments (see tests for example).

Although fragments and queries still need to be parsed individually at some point, they can be recombined at runtime in a way that requires no re-parsing, so this system should eliminate needless (re)parsing of query documents that directly include associated fragment declarations.

Finally, to prove to myself that this is possible, I structured these changes so the implementation of the FragmentRegistry is not automatically included in your JavaScript bundle, unless you choose to import createFragmentRegistry and actually use it. This works because the InMemoryCache implementation knows only about the abstract FragmentRegistryAPI (and how to use it), and does not directly import or depend on any of the code implementing that API (the FragmentRegistry class isn't even exported from the module, for example).

benjamn · 2022-05-26T20:07:09Z

src/core/QueryManager.ts

+    // Performing transformForLink here gives this.cache a chance to fill in
+    // missing fragment definitions (for example) before sending this document
+    // through the link chain.
+    const linkDocument = this.cache.transformForLink(
+      // Use same document originally produced by this.cache.transformDocument.
+      this.transform(queryInfo.document!).document
+    );
+
    return asyncMap(
      this.getObservableFromLink(
-        queryInfo.document!,
+        linkDocument,


Surfacing an important commit message (from the first commit in this PR):

Call cache.transformForLink before each link execution

This is not a disruptive change for InMemoryCache, since it uses the default implementation of transformForLink (inherited from ApolloCache), which simply returns the given document.

If you're using a different ApolloCache implementation that has a custom transformForLink implementation, this new behavior should be more convenient and flexible, but you should probably double-check that your transformForLink method is suitably cached/idempotent, so multiple calls with the same input document return the same (===) transformed document.

jpvajda · 2022-05-26T20:15:35Z

This is cool, and I'm curious if we need to update some documentation on the ability to register named fragments with InMemoryCache and if so is that just a new section in https://github.com/apollographql/apollo-client/blob/main/docs/source/data/fragments.md

benjamn · 2022-05-26T20:17:25Z

@jpvajda You are right about the need for more documentation. Happy to work on that in this PR, since it would help explain the PR as well.

benjamn · 2022-05-26T20:20:02Z

src/cache/inmemory/fragmentRegistry.ts

+    definitions.forEach((node, name) => {
+      if (node !== this.registry[name]) {
+        this.registry[name] = node;
+        this.invalidate(name);


Thanks to this invalidation, if you register new fragments under previously registered names, any queries that used the previous fragment(s) will be invalidated and rerun. In other words, the FragmentRegistry is fully reactive.

jpvajda · 2022-05-26T20:26:06Z

@benjamn that's great I can definitely act as content reviewer and perhaps @StephenBarlow or Rose could do a pass through what we come up as needed.

hwillson

This is really awesome @benjamn! 🎉

hwillson · 2022-05-27T00:06:38Z

src/cache/inmemory/__tests__/fragmentRegistry.ts

+    });
+  });
+
+  it("can register fragments with unbound ...spreads", () => {


The name of this test threw me off a bit on the intent. Maybe can override pre-registered fragments instead?

hwillson · 2022-05-27T00:17:35Z

src/cache/inmemory/inMemoryCache.ts

@@ -514,6 +517,13 @@ export class InMemoryCache extends ApolloCache<NormalizedCacheObject> {
    return document;
  }

+  public transformForLink(document: DocumentNode): DocumentNode {


Definitely not an experimental API anymore!

hwillson · 2022-05-27T00:20:18Z

src/cache/inmemory/__tests__/fragmentRegistry.ts

+      `),
+    });
+
+    // TODO Allow writeFragment to just use fragmentName:"BasicFragment"?


YES! 🎉 That would be super cool.

(doesn't have to be in this PR of course)

hwillson · 2022-05-27T00:32:06Z

src/cache/inmemory/fragmentRegistry.ts

+        if (def) {
+          defsToAppend.push(def);
+        } else {
+          // TODO Warn? Error?


If we end up here does that mean we have fragment spreads that don't have a matching fragment definition defined globally or locally? If so, I'm leaning towards an error (with a nice explanatory error message mentioning the fragment name); might as well fail fast if that's the case.

benjamn · 2022-09-12T21:57:35Z

src/cache/inmemory/inMemoryCache.ts

      if (e instanceof MissingFieldError) {
-        // Swallow MissingFieldError and return null, so callers do not
-        // need to worry about catching "normal" exceptions resulting from
-        // incomplete cache data. Unexpected errors will be re-thrown. If
-        // you need more information about which fields were missing, use
-        // cache.diff instead, and examine diffResult.missing.
+        // Swallow MissingFieldError and return null, so callers do not need to
+        // worry about catching "normal" exceptions resulting from incomplete
+        // cache data. Unexpected errors will be re-thrown. If you need more
+        // information about which fields were missing, use cache.diff instead,
+        // and examine diffResult.missing.
        return null;


I thought about carving out an exception for missing named fragments, like the existing exception for MissingFieldError here, so cache.read could return null in those cases, but I decided against it. Attempting to execute a query with missing fragments seems like a permanent/static error that always needs to be fixed, rather than a behavior influenced by dynamic data (as with missing fields).

This is not a disruptive change for InMemoryCache, since it uses the default implementation of tranformForLink (inherited from ApolloCache), which simply returns the given document. If you're using a different ApolloCache implementation that has a custom transformForLink implementation, this new behavior should be more convenient and flexible, but you should probably double-check that your transformForLink method is suitably cached/idempotent, so multiple calls with the same input document return the same (===) transformed document.

This is just one implementation of a FragmentRegistry that could be provided to InMemoryCache to fill in missing fragment declarations in transformForLink. Learning from past mistakes, we will avoid referring directly to this particular implementation of FragmentRegistry within the InMemoryCache implementation, since that forces the full FragmentRegistry implementation to be bundled even when it is not used.

As long as InMemoryCache uses only this TypeScript interface when handling FragmentRegistry configurations, any implementation can be swapped in without also paying for the bundle size of the default implementation (the FragmentRegistry class).

Since these changes do not include any calls to createFragmentRegistry, the FragmentRegistry class implementation remains optional and thus tree-shakable if unused.

adamesque · 2022-11-07T20:30:19Z

@benjamn It looks like we could use this to populate fragment bodies at runtime by keeping our reference to the FragmentRegistry and calling register as needed to add new fragments. Does that sound accurate?

We're operating in a Webpack Module Federation environment and are thinking about building a client-only @remote directive to allow queries to reference remote fragments — we'd use an Apollo Link to discover and retrieve the fragment bodies at runtime and remove the directive before sending the query over the wire.

This of course blows up today because the cache writers only have a reference to the original unmodified query, and can't find our fragment defn's at runtime after the query data returns from the link.

(Ultimately what we want is some form of first-class client-only directives, but for now it seems like this FragmentRegistry might save us)

jerelmiller · 2022-11-28T22:52:42Z

Hey @adamesque!

Just to confirm I understand, you are writing your queries something like the following:

const QUERY = gql`
  query {
    user {
      ...UserFields @remote
    }
  }
`
// ...

useQuery(QUERY)

Then you want your custom ApolloLink to be able to look for those @remote directives, look up the definition from a remote source, store the fragment definition in the FragmentRegistry, then send the full query, including the fragment definition, to your server. Does this sound accurate?

If so, I think this would work. Looking through this PR, it looks like the writeToStore module looks up fragments before writing to the cache, so storing the fragment in the FragmentRegistry before a cache write should theoretically work.

adamesque · 2022-11-28T23:21:07Z

That's essentially it, yes! Ideally, we wouldn't have to store the fragment reference in the FragmentRegistry — the only reason to do that now is because we can't completely safely modify operation ASTs in a link for the reasons I mentioned. Because pushing into the registry makes a fragment universally available to other operations, I can see valid reasons why (for this feature) we might prefer not to have to use the registry in this way.

But based on my reading of the code and a small spike I put together after writing that comment, this seems to work OK for now. I mostly wanted to check because we're not "pre-registering" these in the traditionally understood way as the PR description indicates, but adding them at any point at runtime.

Thanks very much for your response!

jerelmiller · 2022-11-28T23:24:20Z

You're welcome! Glad to hear you've got something working! I definitely agree that something like the custom directive feature would be much more robust for this kind of thing. Hopefully that's something we can get on our roadmap in the near future!

For now, I see no reason why what you're doing is a bad use of the fragment registry, even if it's less ideal. Really cool stuff!

benjamn added 💡 idea 🧞‍♂️ enhancement 💸 caching labels May 26, 2022

benjamn added this to the Release 3.7 milestone May 26, 2022

benjamn self-assigned this May 26, 2022

benjamn requested a review from hwillson May 26, 2022 19:48

benjamn commented May 26, 2022

View reviewed changes

benjamn added 👩‍🔬 needs-more-tests ⚗️ needs-documentation labels May 26, 2022

benjamn commented May 26, 2022

View reviewed changes

hwillson approved these changes May 27, 2022

View reviewed changes

benjamn force-pushed the named-fragment-registry branch from f14369e to 1d91325 Compare June 28, 2022 15:42

jpvajda assigned alessbell Sep 6, 2022

benjamn force-pushed the named-fragment-registry branch from 1d91325 to 3ffb5eb Compare September 12, 2022 21:03

benjamn commented Sep 12, 2022

View reviewed changes

benjamn force-pushed the named-fragment-registry branch from 72e27a5 to a67eb81 Compare September 15, 2022 16:58

jpvajda mentioned this pull request Sep 20, 2022

Missing AC3 documentation improvements ☔ #6711

Closed

18 tasks

benjamn added 9 commits September 21, 2022 12:55

Allow getFragmentFromSelection to take a FragmentMapFunction.

570dbaa

Prevent isEmpty from recursively calling itself with falsy documents.

c955935

Use InMemoryCache#transformForLink to supply missing fragments.

abecb11

Since these changes do not include any calls to createFragmentRegistry, the FragmentRegistry class implementation remains optional and thus tree-shakable if unused.

Basic tests of configuring InMemoryCache with FragmentRegistryAPI.

52c7f61

When fragmentMap is a function, let it decide whether to throw.

32b4acd

Test error thrown when FragmentRegistry cannot resolve all fragments.

5c6ae24

Bump bundlesize limit to 31.65kB (now 31.63kB).

dc8b8ba

benjamn force-pushed the named-fragment-registry branch from 4036a69 to dc8b8ba Compare September 21, 2022 16:58

benjamn merged commit 05e45c9 into release-3.7 Sep 21, 2022

benjamn deleted the named-fragment-registry branch September 21, 2022 17:04

jpvajda mentioned this pull request Oct 12, 2022

Add createFragmentRegistry documentation #10185

Closed

adamesque mentioned this pull request Jan 10, 2023

RFC: Query/Mutation/Subscription Pre-Processor #10228

Closed

jerelmiller mentioned this pull request Jan 27, 2023

Custom query document transforms #10481

Closed

github-actions bot locked as resolved and limited conversation to collaborators Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow registering named fragments with `InMemoryCache` to support using `...FragmentName` in queries without redeclaring `FragmentName` in every query #9764

Allow registering named fragments with `InMemoryCache` to support using `...FragmentName` in queries without redeclaring `FragmentName` in every query #9764

benjamn commented May 26, 2022 •

edited

Loading

benjamn May 26, 2022

jpvajda commented May 26, 2022

benjamn commented May 26, 2022

benjamn May 26, 2022

jpvajda commented May 26, 2022

hwillson left a comment

hwillson May 27, 2022

hwillson May 27, 2022

hwillson May 27, 2022

hwillson May 27, 2022

hwillson May 27, 2022

benjamn Sep 12, 2022

adamesque commented Nov 7, 2022

jerelmiller commented Nov 28, 2022 •

edited

Loading

adamesque commented Nov 28, 2022

jerelmiller commented Nov 28, 2022

Allow registering named fragments with InMemoryCache to support using ...FragmentName in queries without redeclaring FragmentName in every query #9764

Allow registering named fragments with InMemoryCache to support using ...FragmentName in queries without redeclaring FragmentName in every query #9764

Conversation

benjamn commented May 26, 2022 • edited Loading

Choose a reason for hiding this comment

jpvajda commented May 26, 2022

benjamn commented May 26, 2022

Choose a reason for hiding this comment

jpvajda commented May 26, 2022

hwillson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamesque commented Nov 7, 2022

jerelmiller commented Nov 28, 2022 • edited Loading

adamesque commented Nov 28, 2022

jerelmiller commented Nov 28, 2022

Allow registering named fragments with `InMemoryCache` to support using `...FragmentName` in queries without redeclaring `FragmentName` in every query #9764

Allow registering named fragments with `InMemoryCache` to support using `...FragmentName` in queries without redeclaring `FragmentName` in every query #9764

benjamn commented May 26, 2022 •

edited

Loading

jerelmiller commented Nov 28, 2022 •

edited

Loading