apollographql / apollo-client

:rocket:  A fully-featured, production ready caching GraphQL client for every UI framework and GraphQL server.
https://apollographql.com/client
MIT License
19.38k stars 2.66k forks source link

Release local resolvers back into the link chain #10060

Open jpvajda opened 2 years ago

jpvajda commented 2 years ago

Relates to #8189

If we're already asking folks to change how they pass local resolvers to the ApolloClient constructor, I think we should take this opportunity (AC4) to release local resolvers back into the link chain, which involves the following steps:

Non Breaking

  1. Sending queries into the link chain with @client directives with an option to enable / disable by default. (#10303)
  2. Stripping those @client fields in HttpLink and other terminating links, if not handled earlier. (#10303)

Breaking Change

  1. Providing an @apollo/client/link/local-resolvers package that moves the current LocalState implementation into an easy-to-switch-to ApolloLink, prioritizing backwards compatibility (and equivalent bundle sizes) as much as possible
  2. Providing another link that wraps the graphql package's execute pipeline, like @apollo/client/link/schema currently does, which may indicate an opportunity to reuse @apollo/client/link/schema, though that package pulls in the validate code as well, so we might want a version without that code (TBD)

I want to thank @vigie for pointing out in https://github.com/apollographql/apollo-client/issues/7072 that it's not fair/reasonable of us to deprecate local resolvers AND keep standing in the way of implementing them in the link chain (like apollo-link-state did) by stripping out @client directives before passing queries into the link chain.

Since the link chain is async-friendly, it's tempting to save bundle size by using dynamic import() to fetch parts of the graphql package, though that may not be helpful for code that's always needed during page load.

Originally posted by @benjamn in https://github.com/apollographql/apollo-client/issues/8189#issuecomment-857199932

jpvajda commented 2 years ago

This new issue was a result of our decision in https://github.com/apollographql/apollo-client/issues/8189#issuecomment-857199932

jerelmiller commented 1 year ago

I've opened https://github.com/apollographql/apollo-client/issues/10303 to track points 1 and 2 above.

vigie commented 1 year ago

I'm really excited for this feature, it's going to open up a lot of possibilities. Here's one I've been thinking about of late:

We are (slowly) transitioning client side resolvers to a GQL service across the wire. My first impression was that, from the client perspective this would be easy - just remove @client from the fields that should be sent across, right? That works, however, the wrinkle is that we want to do this conditionaly, after asynchronously checking the status of the feature flag connectToBackendGql or whatever. This has forced the unfortunate pattern of two copies of queries in the codebase - one with @client directives and one without, which we then switch between at runtime based on the value of the feature flag. I couldn't think of a better way. Certainly, a new version of @client that accepts a param would have been neat, but there does not seem to be a way to introduce new operation directives currently.

With the middleware type capabilities of a link, we should easily be able to do this kind of preprocessing of the query, applying @client directives as appropriate after introspecting the GQL endpoints.

I'm sure there are many other possibilities. I just mentioned this as a little motivation. I won't ask for timelines as I know this is a breaking change for 4 which appears to still be in planning. If the core team is looking for help in this area though I may be available, lmk. Cheers.

rajington commented 1 year ago

unfortunate pattern of two copies of queries in the codebase

this does handle the, likely pretty realistic, use case that the operation changes when migrating. it also lets you incrementally migrate portions of an operation vs. an operation-level flag. this capability could be implemented using a split link or some middleware that performs AST conversions

jerelmiller commented 1 year ago

@vigie you might be interested in our custom document transforms feature that will land in 3.8 (final API TBD). This might give you a way to handle this migration in a way that doesn't require you to create 2 distinct queries.

That being said, will you have the ability to load your feature flags ahead of time before executing the queries? The transforms are likely to only be synchronous for technical reasons, so to take advantage, you'd need to have those flags loaded before-hand.

v4 is probably still a ways off, so this might be a nice middleground until we can land all our breaking changes in the next major. Let me know what you think!

tobiasBora commented 1 year ago

I’m lost: are local resolvers un-deprecated? If they are still deprecated, how can we implement useMutation, useful for abstraction, reusability, and debugging purposes? (I tried to ask a question here)

phryneas commented 1 year ago

@tobiasBora they are not un-deprecated. Moving them out of the core into a link would be a first step in removing them/not shipping them to all of our users that don't use them.

In your example, you should probably use a reactive variable instead.

tobiasBora commented 1 year ago

@phryneas Thanks for your message. Regarding reactive variables, I have two issues with them:

  1. as far as I understand they cannot work to normalize nested data, right? For instance, if I set it to:
    myVariable([
          {name: "Cookies", ingredients: [
              {name: "Sugar", quantity: "100g"},
              {name: "Chocolate", quantity: "250g"}
          ]},
          {name: "Choux", ingredients: [
              {name: "Eggs", quantity: "2"},
              {name: "Flour", quantity: "250g"}
          ]},
    ])

    then this will be stored as one nested object, which is of little interest in, e.g., react, as changing a deeply nested element would redraw the whole interface, leading to a lagging interface.

  2. as far as I understand, they offer no way to create a new useMutation to abstract the actual logic used to apply the change, which means that it is harder to move from a local store to a remove store.

Am I missing something, or do you have some solutions to address these issues? You can also see some attempts of mine here.