-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
Custom document transforms #10509
Custom document transforms #10509
Conversation
🦋 Changeset detectedLatest commit: 46748ba The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
/release:pr |
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.
From my side, it looks good. I have left some optional comments, but I would also approve as-is :)
Aside: the |
/release:pr |
A new release has been made for this PR. You can install it with |
It's been a while since there has been movement on this PR. There are lots of different things that we discussed internally to figure out a path forward so here is my attempt to document the latest thinking and potential changes. Simplifying the APIThe current implementation has both a This leads into the next point: Caching transform outputOne of the hold-ups in this PR was figuring out how best to cache the result of the transform to avoid recomputing the output document more than necessary. Transformations can be expensive, especially for large query documents, so ensuring this feature is performant is a must. One of the ideas we discussed was trying to auto-cache all transforms out-of-the-box. This solution however has a lot of edge cases. This also makes it complicated for a developer to tell Apollo Client when to invalidate the cache for a query document if a transform needs to be recomputed. In place of auto-caching, we discussed providing a utility to each transform function that would allow the user to determine which documents to cache, complete with an API to invalidate a document. While this is certainly feasible, we think it might be best to save some bytes and instead allow the user to use a cache mechanism that suites them best. A popular way to achieve this is through memoization. There are a vast number of memoization libraries out there, so this allows the developer to use tools that they are already familiar with. We think memoization covers most use cases that a user might have. If a user needs a more than memoization, the user has the freedom to implement a cache strategy that works best for them. This is totally optional and purely a performance benefit, though we plan to encourage the use of memoization when possible to ensure apps are performant. As such, we plan to make sure the API of the transform function can behave nicely with memoization. We may opt to add a general purpose utility later, but are ok with leaving this up to the developer for now. Additional use casesIn the future, Apollo may provide some nice utilities to persist and extract query documents during build-time. As such, we want want to make sure the API of this document transform feature can be easily utilized to transform documents during a build. I'm proposing the following changes to better faciliate this:
import { DocumentTransformer } from '@apollo/client';
const transformer = new DocumentTransformer();
transformer
.add(transform1)
.add(transform2);
new InMemoryCache({
documentTransforms: transformer
}); By allowing the user to create the transformer instance outside of the cache, this allows the document transformer to be exported anywhere in an app, which can be useful to isolate the transformer for a build time feature.
const transforms1 = new DocumentTransformer();
transforms1.add(...);
const transforms2 = new DocumentTransformer();
transforms2.add(...);
const transformer = transforms1.concat(transforms2); Allowing a user to compose transformers together allows someone to organize or split document transforms however they wish.
import { createDefaultTransformer } from '@apollo/client';
// Allow users to disable typename
// This also allows InMemoryCache to configure this transform based on its addTypename option
const transformer = createDefaultTransformer({ addTypename: false }); |
ed81e55
to
4c99954
Compare
0913a64
to
fc93384
Compare
Making large changes that are completely different than the initial attempt, so the old review makes no sense anymore.
…e link chain This undoes some of the work in #10346. With the new solution presented in this branch, I see no need for the new options.
4c7f1cd
to
bf9f535
Compare
…cache keys instead
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 pushed a few simplifying changes in ee0b4ae, but overall I am happy with the state of this PR (especially for a beta release). Thanks @jerelmiller!
src/core/DocumentTransform.ts
Outdated
type TransformFn = (document: DocumentNode) => DocumentNode; | ||
|
||
interface DocumentTransformOptions { | ||
cache?: boolean; | ||
getCacheKey?: (document: DocumentNode) => DocumentTransformCacheKey; |
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've found it useful in the past for cache-key-generating functions like getCacheKey
to have the option of refusing to generate a key when the cache should be skipped/ignored. Specifically, getCacheKey
could return DocumentTransformCacheKey | undefined
, with undefined
indicating to skip the cache for this document. That decision could be made dynamically based on the contents of the document
, for example.
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 made this change as part of ee0b4ae.
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.
Yes that makes a ton of sense. Thanks so much for this update!
/release:pr |
A new release has been made for this PR. You can install it with |
transformDocument(document: DocumentNode) { | ||
// If a user passes an already transformed result back to this function, | ||
// immediately return it. | ||
if (this.resultCache.has(document)) { |
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 get the reasoning for the resultCache
, but we have to make sure to mention in the docs that a transform(document) === transform(transform(document))
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.
This looks good to me - no further complaints from my side :)
Closes #10481
This adds the ability to specify custom GraphQL document transforms. The default behavior is fully backwards compatible with the client today and allows custom caches (i.e. non
InMemoryCache
implementations) to operate on the transformed document, an improvement from the first iteration of this feature.Behavior
Document transforms are run before reading from the cache, before local resolvers are used, and before the document is sent through the link chain. We run these as early as possible throughout the client to allow a custom transform an opportunity to make changes to the query document before it reaches the rest of the system. This means these situations are properly handled:
@client
directive to a field from the transform will properly be sent through to local stateTransforms can contain runtime conditions that may change the resulting document between requests. Because of this, custom transforms are also run again before every request is made. For example, if we use
client.watchQuery(...)
, then callobservable.refetch()
on the returned observable, the custom transforms are run again before therefetch
is executed.When a custom transform is added to the client, we run the document through the
cache.transformDocument
hook twice; once before the custom transforms are executed, and once again afterward. This allows custom transforms to transform a full query after the cache has made changes, but also gives the cache a chance to fill in any gaps left by the custom transform. This is especially useful for caches that provide a fragment registry, such asInMemoryCache
. By running the cache transforms before the custom transforms, custom transforms have a chance to modify the fragment definitions added to the query from the fragment registry. Since custom transforms have the freedom to make any change necessary to the document, such as adding additional field selections or fragment spreads, the cache transforms run again to fill any gaps left over from the custom transform.It's important to note that transforms are run before we read data from the cache. This ensures the cache transforms have the freedom to make any modifications necessary and be confident the resulting data will be returned against the transformed query.
Transforms are also run before we send any data to local resolvers. A custom transform may add a
@client
directive to any field, so we want to ensure we can catch that and send it through local state.API
To register a custom transform, create a document transform using the
DocumentTransform
class and set it onApolloClient
Document transforms are automatically cached. If your transform may rely on a runtime condition, you can provide your own cache key that will only re-run your transform when the cache key changes. We highly recommend using the passed
document
argument as part of your custom cache key:If you want to conditionally cache the document based on its contents, you may return
undefined
fromgetCacheKey
to disable caching for that document.To disable caching altogether to allow for your transform function to always be re-run, set the
cache
option tofalse
:You can combine transforms using the
.concat
function on the transform. This allows you to form a "chain" of transforms. Thecache
option is respected for each transform in the chain.You may also conditionally run a set of transforms depending on some condition. Use the
.split
static function on theDocumentTransform
constructor to handle this.split
also accepts a second transform to structure this like an if/else:This PR also undoes some work done in #10346 that provided a new
transformQuery
option passed to theApolloClient
constructor, released in v3.8.0-alpha.2. With the work in this PR, I just don't see the need for this option. The work to remove@client
directives inHttpLink
andBatchHttpLink
remains as that is important work on the goal to move the processing of@client
to the link chain.Checklist: