Skip to content
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 query document transforms #10481

Closed
jerelmiller opened this issue Jan 27, 2023 · 17 comments · Fixed by #10509
Closed

Custom query document transforms #10481

jerelmiller opened this issue Jan 27, 2023 · 17 comments · Fixed by #10509

Comments

@jerelmiller
Copy link
Member

jerelmiller commented Jan 27, 2023

This is an extension of #10228 originally opened by @nyteshade.

Background

#10228 laid out a well thought-out proposal for defining a custom preprocessor to be able to transform queries before they run through Apollo core. The idea of a preprocessor was necessary because of the order in which Apollo runs local resolvers/cache read functions for @client directives in a query relative to the link chain.

Initially I had thought this could be resolved simply by allowing @client directives to make it to the link chain, where a custom link could then make behavioral decisions based on the query at that point. While that work is a necessary step in the effort to move local resolvers into the link chain, I've come to realize this solution is incomplete as doing work in a link is too late in the process.

Without repeating too much detail from #10228, the goal with the @persistent directive was to determine whether to convert that directive to an @client directive, or drop the directive altogether. The problem with my initial solution is that @client fields run local resolvers/cache read functions in Apollo core, before they ever reach the link chain. If you try and perform the work necessary to transform the query document in an Apollo link, the work is performed too late as the local resolvers have already been run by that point.

Proposal

After reading through the proposal several more times, and with more discussion on the topic, I'm understanding the primitive ask is the ability to define custom GraphQL document transforms. We already perform document transforms in Apollo core (e.g. stripping out @client and adding __typename), but currently have no way for users to register their own.

The Cache abstraction currently provides 2 extension points for cache implementations to make query document transforms:

  • transformDocument - This is intended to make static changes to the query, such as adding __typename to the document. This transformed document is heavily cached to avoid performing this work more than necessary. In other words, this function will only be called once per unique GraphQL query document.
  • transformForLink - This is run once per query and allows the cache to tweak the document before the request is sent. InMemoryCache cache takes advantage of this hook to fill in fragments from the fragment registry in the query.

Given these hooks, I propose we add an option to allow registration of query transforms with InMemoryCache. We'll want to separate the idea of a "static" transform and "dynamic" transform so that we can run the appropriate transforms in transformDocument and transformForLink. This allows allows the user to make the determination on how often the query transformation is run.

API

Here is a "back of the napkin" idea on what this API might look like to provide a starting point for discussion. I expect the names and API to evolve before released. This idea is meant to lay out the pieces involved:

function customStaticDocumentTransform(document: DocumentNode): DocumentNode {
  // perform some transformation on the document
  return transformedDocument;
}

function customDynamicDocumentTransform(document: DocumentNode): DocumentNode {
  // perform some transformation on the document
  return transformedDocument;
}

const cache = new InMemoryCache({
  // transforms run as a part of `transformDocument`
  documentTransforms: [customStaticDocumentTransform],

  // transforms run as a part of `transformForLink`
  documentTransformsForLink: [customDynamicDocumentTransform]
});

These transforms would be run in order they are defined, where index 0 would be run first.

How is this proposal different than #10228?

This proposal is very similar to #10228 with the notable exceptions that query transforms are defined in InMemoryCache and don't require an Apollo Link or to call a registration function on ApolloClient. I thought it would be easier to lay out the idea in totality using a separate issue rather than trying to define all of this in a comment in the original RFC.

What this means for #10346

While #10346 is now less relevant to this proposal, its still an important step in the ability to be able to move local state to an Apollo Link. There is however one change I'd like to make. I'd like to remove the transformQuery option passed to the ApolloClient constructor as it simply doesn't meet the goal that the original RFC was intended to address. Until we get local resolvers in the link chain, I don't see much use in this option.

What this proposal is not

There is an ask in our feature requests for the ability to define custom directives (apollographql/apollo-feature-requests#166). This updated proposal is not meant to address this feature request as its a different set of concerns. This proposal is strictly the ability to transform a query document before it runs through the process of making the request.

@jerelmiller
Copy link
Member Author

@nyteshade, I'd love to get your feedback here and see if this proposal better addresses your original goal you laid out in #10228.

@nyteshade
Copy link

@jerelmiller I think this is mostly sound. I only really have one question upon seeing what has been laid out. I want to make sure that our transform function runs for every query and mutation and is not overly cached; does this mean I would place the corresponding function in the documentTransformsForLink array?

On an adjacent note, I'd like to say that custom query transforms will go a long way to helping companies not need to monkey patch the client and provide a lot of opportunities to reduce code elsewhere in companies' codebases. Thank you for considering it.

@jerelmiller
Copy link
Member Author

jerelmiller commented Jan 27, 2023

@nyteshade That's correct! Looking through the codebase, it looks like the result from cache.transformForLink is not cached, so the documentTransformsForLink would be the ideal place for the work you're trying to do with @persistent.

Glad to hear this is a lot more in line with your expectation! Thanks so much!

@nyteshade
Copy link

Thanks so much for taking the time to understand the issues and being open to the potential it brings.

@adamesque
Copy link

I greatly appreciate your work on this proposal, @jerelmiller. I'm curious what the delta is between this and full-fledged custom client directive support — in our case, for our custom @remote directive, we'd need an async-capable execution context similar to a Link and perhaps access to the configured client instance. A single point of composition would help with portability/sharing transforms as well (as opposed to having to plug in the two transforms separately in the cache config). Is it worth thinking about some of these concerns now?

@jerelmiller
Copy link
Member Author

@adamesque that is a great question!

I view this proposal as strictly the ability to transform query documents from some initial value to another before the query is sent to the server. Two examples in this repo are 1) adding __typename to each field and 2) stripping @client fields from the document before its sent to the server (both of which I plan to reimplement in terms of this API).

The only reason I see a need for this to exist is because of the fact that @client directives are processed in core. If you want to provide something that interacts with local state (like the @persistent needs to), there is no way to do it currently. To be honest, it's likely 99% of apps won't need this API.


I've thought about this a lot the last couple days and I've actually been wondering... is there any reason we couldn't do custom directives in the link chain today? I'd still like to do a proof of concept to try this out, but I'd be curious if you can think of any reasons why ApolloLink wouldn't be suited for this. I could see a future where we provide an out-of-the-box ApolloLink that provides a nice API to define custom directives, but I think the pieces are there to be able to do this:

  • You can intercept requests before they reach the terminating link
  • You can interact with the data returned from a terminating link and modify if need be
  • It works with async operations
  • You can modify the query document before it reaches the terminating link

Thoughts?

@dylanwulf
Copy link
Contributor

dylanwulf commented Feb 3, 2023

@jerelmiller Hi, I haven't really been involved in this thread but I've tried doing query transformations in the link chain before so I thought I'd share my input. Transforming the query document itself isn't terribly difficult with graphql-js's visit function. The hard part that I haven't figured out is how to merge all the result data together. I feel like I need an entire graphql server implementation just to put all the result data where it's supposed to go.

@jerelmiller
Copy link
Member Author

jerelmiller commented Feb 3, 2023

@dylanwulf thanks for chiming in! Glad to hear you've got some practice with this in some form.

The hard part that I haven't figured out is how to merge all the result data together. I feel like I need an entire graphql server implementation just to put all the result data where it's supposed to go.

Definitely understand this, which is why I think providing an out-of-the-box ApolloLink that handles the hard stuff for you would make a lot of sense. Just trying to understand if there is a severe limitation in ApolloLink that would prevent us from using this abstraction to provide an API for custom directives (besides writing the hard stuff yourself).

@dylanwulf
Copy link
Contributor

dylanwulf commented Feb 3, 2023

@jerelmiller Here is a custom client-only directive I made for our app. Our backend has a restriction that it can only return a max of 250 results in one request, so I wrote this @fetchAll directive to automatically fetch all results and combine them together. I created this gist while we were still on apollo-client v2 so the imports might look a bit off but the rest is largely unchanged
https://gist.github.com/dylanwulf/3c2997a7a66b2fceeb9a9f61c3b7c5a6

@jerelmiller
Copy link
Member Author

Oh sweet. Thanks for sharing @dylanwulf!

@adamesque
Copy link

adamesque commented Feb 3, 2023

I've thought about this a lot the last couple days and I've actually been wondering... is there any reason we couldn't do custom directives in the link chain today? I'd still like to do a proof of concept to try this out, but I'd be curious if you can think of any reasons why ApolloLink wouldn't be suited for this. I could see a future where we provide an out-of-the-box ApolloLink that provides a nice API to define custom directives, but I think the pieces are there to be able to do this:

  • You can intercept requests before they reach the terminating link
  • You can interact with the data returned from a terminating link and modify if need be
  • It works with async operations
  • You can modify the query document before it reaches the terminating link

Great question! The main issue I ran into was that after data is returned from the terminating link, the cache writer writes to the cache using the original untransformed query document. In our case, since our custom directive fetches fragment bodies that don't appear in the original document, this means the cache writer explodes when walking the doc to write the results, since the fragment spread refers to a fragment that only exists in the transformed doc. (We work around this by adding our fetched fragment bodies to the Fragment Registry)

I was thinking this would prohibit using a link to implement any kind of custom directive, but I hadn't considered that the correct way to do that in most cases would be intercept the result and transform it to match the original untransformed query structure.

I don't know if there are other use-cases where cache writing would break if the cache isn't aware of the result of a document-transforming Link and the original document might be considered invalid. Could something like Relay's @arguments/@argumentDefinitions directives currently be implemented in a link? You could definitely perform the variable replacement before handing off to the terminal link, but would the cache or something else break when attempting to write fragment data with a variable not present in the operation signature?

@jerelmiller
Copy link
Member Author

@adamesque you bring up some great points! I'm going to do some more exploration in the coming days to figure out how the cache would interact. Appreciate your thoughts as it gives me more to think about!

@bignimbus bignimbus linked a pull request Feb 7, 2023 that will close this issue
3 tasks
@adamesque
Copy link

We've been discussing another concrete use-case for client directives, to reduce our reliance on plumbing operation variables through query fragments just to turn on/off fields under experimentation (A/B testing).

Our idea was to implement a client-only set of experiment-aware skip/include directives, to transform a document like this:

query MyQuery {
  foo {
    ...MyFooFragment
    baz
  }
}

fragment MyFooFragment on Foo {
  bar @includeInExperiment(name: 'my_experiment', value: 1)
}

into a vanilla GraphQL @include directive depending on a new operation variable whose value would be supplied by the custom Link implementing the directive, like so:

query MyQuery($my_experiment_1: boolean) {
  foo {
    ...MyFooFragment
    baz
  }
}

fragment MyFooFragment on Foo {
  bar @include(if: $my_experiment_1)
}

with operation variables:

{
  "my_experiment_1": true
}

We can't build this today in a Link because I believe it would break cache reads. The cache wouldn't have the transformed document, and so wouldn't understand that it could potentially skip the bar field (as currently happens here) and would likely treat it as a cache miss.

I was hoping that we could use the APIs from this proposal/PR to resolve this but I don't think that's possible either since everything hinges upon the ability to not only transform the document, but also to supply operation variables per-read (whether from cache or from network).

We starting thinking through this while imagining how calling client.readFragment with the fragment and proposed experiment directive above would work, and quickly realized that we didn't think it could. I think this is why I was asking for a "link-like" API we could use to define operation transforms at the cache level.

@jerelmiller
Copy link
Member Author

Hey @adamesque 👋

This is a really interesting use case.

We've been having a lot of discussions internally about this API and we have some ideas on how to make it more powerful, including some built-in caching mechanisms to avoid having to recompute the document more than is necessary . I'll be sharing some more about this idea soon. This use case gives me a good stress test to see if we could execute on something like this given the direction we want to go with the new API.

@adamesque
Copy link

Wonderful, thanks again!

@jerelmiller
Copy link
Member Author

This issue has been completed with #10509.

If anyone would like to try this out and provide feedback, this has made it into our first v3.8.0 beta release. Install via:

npm install @apollo/client@beta

For now, the API is documented in the PR. We plan to document this in the Apollo docs in the coming days/weeks. I'm excited to get this in your hands!

@github-actions
Copy link
Contributor

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
For general questions, we recommend using StackOverflow or our discord server.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants