kamilkisiela / apollo-angular

A fully-featured, production ready caching GraphQL client for Angular and every GraphQL server 🎁
https://apollo-angular.com
MIT License
1.5k stars 311 forks source link

Apollo Angular 4.0 `QueryRef.fetchMore` parameter typings remove `updateQuery` property #1810

Closed KeithGillette closed 2 years ago

KeithGillette commented 2 years ago

Describe the bug Upgrading from apollo-angular@3.0.1 to apollo-angular@4.0.1 produces TS2345 error on code which passes an updateQuery property to the fetchMoreOptions parameter of a QueryRef.fetchMorecall.

To Reproduce Previously, the QueryRef.fetchMore fetchMoreOptions parameter was typed with FetchMoreQueryOptions & FetchMoreOptions but Apollo Angular 4 removed FetchMoreOptions and with it the updateQuery property on the parameter.

Expected behavior The method signature should not change, since the underlying Apollo Client ObservableQuery.fetchMore method still has updateQuery in its signature.

Environment:

├── @angular/cli@14.1.3 
├── @angular/core@14.1.3 
├── @apollo/client@3.6.8 
├── apollo-angular@4.0.1 
├── barrel-maintainer@1.5.3
├── graphql@16.5.0 
└── typescript@4.7.4 

Additional context That pretty much sums it up.

KeithGillette commented 2 years ago

Finding that this typing issue still exists in release 4.1.0, I investigated further and found that while updateQuery is still supported Apollo Client 3, it is deprecated and will be removed in the next major version of Apollo Client, so while its removal in Apollo Angular 4 may be somewhat premature, it is inevitable. I refactored our use of updateQuery (merging paginated results) to use the Apollo Client recommended solution of adding a field policy with a merge function.

alfaproject commented 2 years ago

@KeithGillette I have a dynamic helper that creates a dynamic updateQuery to avoid repetition, something like this:

      updateQuery: (prev, { fetchMoreResult }) => {
        if (!fetchMoreResult) {
          return prev;
        }

        const newEdges = fetchMoreResult[this.connectionFieldName].edges;
        if (newEdges.length === 0) {
          return prev;
        }

        return {
          [this.connectionFieldName]: {
            ...fetchMoreResult[this.connectionFieldName],
            edges:
              this.direction === Direction.ASCENDING
                ? [...prev[this.connectionFieldName].edges, ...newEdges]
                : [...newEdges, ...prev[this.connectionFieldName].edges],
          },
        };
      },

Are you telling me that we now need to go to our cache adaptor and add type policy for each individual connection. We have hundreds of them and each one is 'owned' by a particular service which is agnostic to the cache o.o

KeithGillette commented 2 years ago

Hi, @alfaproject — Well, so far as I can see, updateQuery is still available on ObservableQuery in Apollo Client 3.x and not marked in the code as deprecated, so I think you could continue using it with Apollo Angular 4 with suitable @ts-ignore comments, but since I haven't seen anything to contradict the statements from the Apollo team that I linked to in my last message indicating that updateQuery will be removed in Apollo Client 4 and the fetchMore documentation has been updated to remove reference to updateQuery in favor of field policies, I do think that you'll eventually have to update your code to use merge functions on field policies, one for each field that needs your dynamic updateQuery logic. Note that field policy merge functions can be generic with respect to the data type, just as your updateQuery function example is.

jwrascoe commented 2 years ago

@KeithGillette I also do the same as you with respect to a dynamic helper.... I cant seem to figure out get a simple policy merge working let alone a dynamic one have you?

For now I put in the a @ts-ignore and im working fine on the latest but somehow need to figure out this policy merge.

My existing super simple merge that works fine.

    updateQuery: (prev, { fetchMoreResult }) => {
      if (!fetchMoreResult) {
        return prev;
      }
      fetchMoreResult.allProducts.edges = [
        ...prev.allProducts.edges,
        ...fetchMoreResult.allProducts.edges
      ];
      return fetchMoreResult;
    }
  }

I would think something like this would work.. but incoming is always null and not iterable

cache: new InMemoryCache({
  typePolicies: {
    Query: {
      fields: {
        allProducts: {
          keyArgs: false,
          merge(existing = [], incoming) {
            console.log('I need to figure out how to merge');
            return [...existing, ...incoming];
          }
        }
      }
    }
  }
})
KeithGillette commented 2 years ago

@jwrascoe — I'm not sure why the incoming argument would always be null in your case. The field policy merges we're using are either integrating skip/limit or page-based pagination results, not relay-style cursors. We're using basically a copy/paste of the sample merge functions on the Apollo documentation.

jwrascoe commented 2 years ago

@KeithGillette -- yes its strange, I have been using Apollo since its early inception against a postgres via PostGraphile and have never had a situation like this before, perhaps its related to how edges are returned or that I am calling only with a limit and offset from a cursor

So strange...

--Forgive me on the formatting.. not sure why its getting messed up in git

fetchDataMore() { this.productQuery.fetchMore({ variables: { limit: 8, offset: this.searchResults.edges.length } }); }

query allProductsByClientIdForSearchMore( $offset: Int $limit: Int ) { allProducts( orderBy: NAME_ASC offset: $offset first: $limit

) { totalCount pageInfo { hasNextPage endCursor } edges { cursor node { id name price } } } }