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

Release local resolvers back into the link chain #10060

Open
jpvajda opened this issue Aug 31, 2022 · 8 comments
Open

Release local resolvers back into the link chain #10060

jpvajda opened this issue Aug 31, 2022 · 8 comments

Comments

@jpvajda
Copy link
Contributor

jpvajda commented Aug 31, 2022

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. (Add option to allow @client directives to make it to the link chain #10303)
  2. Stripping those @client fields in HttpLink and other terminating links, if not handled earlier. (Add option to allow @client directives to make it to the link chain #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 #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 #8189 (comment)

@jpvajda
Copy link
Contributor Author

jpvajda commented Aug 31, 2022

This new issue was a result of our decision in #8189 (comment)

@jerelmiller
Copy link
Member

I've opened #10303 to track points 1 and 2 above.

@vigie
Copy link

vigie commented May 8, 2023

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
Copy link
Contributor

rajington commented May 8, 2023

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
Copy link
Member

@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
Copy link

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
Copy link
Member

@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
Copy link

@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.

@jerelmiller jerelmiller removed this from the Release 4.0 milestone Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants