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

fetchMore's updateQuery option receives the wrong variables #2499

Closed
jamesreggio opened this issue Nov 6, 2017 · 53 comments
Closed

fetchMore's updateQuery option receives the wrong variables #2499

jamesreggio opened this issue Nov 6, 2017 · 53 comments

Comments

@jamesreggio
Copy link
Contributor

jamesreggio commented Nov 6, 2017

The updateQuery method passed to ObservableQuery.fetchMore is receiving the original query variables instead of the new variables that it used to fetch more data.

This is problematic not so much because it breaks the updateQuery method contract (which is bad, but clearly doesn't seem to impact many people), but rather because when the results from fetchMore are written to the store in writeQuery, the old variables are used. This cases issues, say, if you were using an @include directive and the variable driving its value changes.

Repro is available here: https://github.com/jamesreggio/react-apollo-error-template/tree/repro-2499

Expected behavior

nextResults.variables shows petsIncluded: true and the pets list gets displayed.

Actual behavior

nextResults.variables shows petsIncluded: null and the pets list is not displayed because the write never happens due to the @include directive being evaluated with the old variables.

Version

@jamesreggio
Copy link
Contributor Author

jamesreggio added a commit to jamesreggio/apollo-client that referenced this issue Nov 6, 2017
@jamesreggio
Copy link
Contributor Author

So, my hack above doesn't actually resolve the problem :-/

To be clear, there are two — and possibly three — related issues at play here:

  1. nextResults has the wrong variables — easy to fix.
  2. The value returned by the updateQuery function is written to the cache with old variables. Perhaps this is okay, and perhaps it's not. I'm worried that if the new variables are used and are different than the original variables, the original observer will not 'see' the updated query data.
  3. If it's determined that the original variables ought to be used during writeQuery, it then seems impossible to force the inclusion of fields that were originally excluded via a variable-driven directive.

@jbaxleyiii jbaxleyiii added this to the Post 2.0 bugs milestone Nov 13, 2017
@jbaxleyiii jbaxleyiii self-assigned this Nov 13, 2017
@jbaxleyiii
Copy link
Contributor

@jamesreggio as always, thank you for the thoughtful research and repo! I'll get on it ASAP!

@stale
Copy link

stale bot commented Dec 4, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

@jamesreggio
Copy link
Contributor Author

Here's a GIF to keep this open.

gif

@stale stale bot removed the no-recent-activity label Dec 4, 2017
@leoasis
Copy link
Contributor

leoasis commented Dec 11, 2017

I think I've run into this same issue while trying to do pagination. I'm trying to do

fetchMore({
  variables: {
    page: variables.page + 1
  }
}, ...);

where variables come from data.variables but those never seem to get updated, they are always the initial variables. This causes the code to always attempt to fetch the second page.

@padupuy
Copy link

padupuy commented Jan 15, 2018

Any update ?
I use fetchMore function to create an infinite scroll with the FlatList Component (React Native).
As @jamesreggio said, the variables seems not be updated and my infinite scroll turns into an infinite loop of the same query result.

I also notice that queryVariables is null inside the updateQuery callback.

List of the packages I use :

  • "apollo-cache-inmemory": "1.1.5"
  • "apollo-cache-persist": "0.1.1"
  • "apollo-client": "2.2.0"
  • "apollo-link": "1.0.7"
  • "apollo-link-error": "1.0.3"
  • "apollo-link-http": "1.3.2"
  • "apollo-link-state": "0.3.1"
  • "graphql": "0.12.3"
  • "graphql-tag": "2.6.1"
  • "react": "16.0.0"
  • "react-apollo": "2.0.4"
  • "react-native": "0.51.0"
const { data } = this.props;

<FlatList
      data={data.feed}
      refreshing={data.networkStatus === 4}
      onRefresh={() => data.refetch()}
      onEndReachedThreshold={0.5}
      onEndReached={() => {
        // The fetchMore method is used to load new data and add it
        // to the original query we used to populate the list
        data.fetchMore({
          variables: { offset: data.feed.length + 1, limit: 20 },
          updateQuery: (previousResult, { fetchMoreResult,  queryVariables}) => {
            // Don't do anything if there weren't any new items
            if (!fetchMoreResult || fetchMoreResult.feed.length === 0) {
              return previousResult;
            }

            return {
              // Concatenate the new feed results after the old ones
              feed: previousResult.feed.concat(fetchMoreResult.feed),
            };
          },
        });
      }}
    />

@pleunv
Copy link
Contributor

pleunv commented Jan 24, 2018

Encountering the same issue here. refetch({ ...variables }) updates the initial variables, fetchMore({ variables }) does not. If you are not getting the page index/cursor information in your response it's a bit difficult to get/keep a reference to this now (still trying to find a good approach). Is this intended behavior, and would there be an advisable workaround?

@pleunv
Copy link
Contributor

pleunv commented Jan 25, 2018

In case this might interest someone, I'm currently working around this by passing the necessary paging parameters combined with an updater function from the child component calling the fetchMore, something like this:

const withQuery = graphql(MY_LIST_QUERY, {
  options: ({ pageSize, searchText }) => ({
    variables: {
      pageIndex: 1,
      pageSize,
      searchText: searchText
    }
  }),
  props: ({ data }) => ({
    data: {
      ...data,
      fetchMore: ({ pageIndex, updatePageIndexFn }) => {
        const nextPageIndex = pageIndex + 1;
        updatePageIndexFn(nextPageIndex);

        return data.fetchMore({
          variables: {
            pageIndex: nextPageIndex
          },
          updateQuery: (previousResult, { fetchMoreResult }) => ({
            items: [
              ...previousResult.items,
              ...fetchMoreResult.items
            ]
          })
        });
      }
    }
});

class ListComponent extends React.Component {
  constructor(props) {
    super(props);
    this.pageIndex = props.data.variables.pageIndex;
  }

  handleLoadMore = event => {
    event.preventDefault();
    this.props.data.fetchMore({
      pageIndex: this.pageIndex,
      updatePageIndexFn: index => this.pageIndex = index
    });
  }

  render() {
    return (
      <React.Fragment>
        <ul>{this.props.items.map(item => <li>{JSON.stringify(item, null, 2)}</li>)}</ul>
        <a href="" onClick={this.handleLoadMore}>Load More</a>
      </React.Fragment>
    );
  }
}

const ListComponentWithQuery = withQuery(ListComponent);

edit: What I initially assumed would work is this:

const withQuery = graphql(MY_LIST_QUERY, {
  options: ({ pageSize, searchText }) => ({
    variables: {
      pageIndex: 1,
      pageSize,
      searchText: searchText
    }
  }),
  props: ({ data }) => ({
    data: {
      ...data,
      fetchMore: () => data.fetchMore({
        variables: {
          pageIndex: data.variables.pageIndex + 1
        },
        updateQuery: (previousResult, { fetchMoreResult }) => ({
          items: [
            ...previousResult.items,
            ...fetchMoreResult.items
          ]
        })
      })
    }
});

@dehypnosis
Copy link

dehypnosis commented Feb 22, 2018

Is this intended?
data.variables not updated on fetchMore called with new variables

@ZiXYu
Copy link

ZiXYu commented Mar 16, 2018

still got this issue.. hard to do pagination with refetch now

@mefjuu
Copy link

mefjuu commented Apr 5, 2018

Any chances to fix that 5-months old issue? It seems to be quite significant disfunction. A bit funny considering an activity of Apollo team on various React confs and theirs effort to promote Apollo integrations...

@stefanoTron
Copy link

same here, I'm using [email protected]

@Billy-
Copy link

Billy- commented Apr 16, 2018

@jbaxleyiii any update on this at all please?

Kind regards,

@BipinBhandari
Copy link

I am amazed how such a critical issue on Apollo is not being addressed! How are current apollo users handling cursor based pagination? I am having hard time coming with a hack to solve this issue.

Can anyone suggest a way to get cursor based pagination working, at least for now?

@himerus
Copy link

himerus commented May 22, 2018

I'd love to see some help/movement on this issue as well.

Using latest (2.3.1) apollo-client.

Attempting to use the new component, and implement similar to mentioned above. I've been able to get the single 'load more' button to work per the examples on the 'pagination' page, and get those results to either add to the end of the previous result set (infinite scroll style) or replacing the previous result set (Next page style).

However, as soon as I try to implement numbered pages, and try passing in a page number to do calculations with to configure the actual offset, i continually get ObservableQuery with this id doesn't exist errors.

@himerus
Copy link

himerus commented May 22, 2018

@ZiXYu did your comment intend to say you had to do pagination with refetch because if this issue? Instead of hard?

I was considering it as a path to take, but hadn't seen much in the way of documentation on it.

@JeffersonFilho
Copy link

This issue still happening on "apollo-client": "^2.3.7"

@gcangussu
Copy link

gcangussu commented Sep 12, 2018

Same issue on apollo-client": "^2.4.2". Happens even if I set fetchPolicy to no-cache.

edit: There was no problem with Apollo, the bug happened because we were mutating an object that was passed by props. So it was the same object, mutated, but still the same reference.

@amannn
Copy link
Contributor

amannn commented Sep 18, 2018

I'm seeing the same issue when I invoke a fetchMore call and change the route so the component that has invoked the call unmounts. I've seen this across various apps that use react-apollo.

You can also see it here in action:

  1. Go to https://spectrum.chat/react
  2. Scroll to the bottom
  3. Before the next page arrives, click on the support button in the main navigation
  4. The error appears in the console

spectrum.chat runs on [email protected].

@ebenoist
Copy link

ebenoist commented Nov 9, 2018

Same here on 2.4.3 -- I'll try and work around this for now.

@gcangussu
Copy link

I've just updated my old comment, there was no problem with the Apollo library in my case. After hours of debugging I've discovered that one object was being mutated and expected to trigger a different render. Off course it did not, Apollo thought it was the same object as before.

@myhendry
Copy link

myhendry commented Nov 23, 2018

Using HOC, it seems to work. The offset variable can be changed with fetchMore. the limit variable i passed in through HOC. https://stackoverflow.com/questions/53440377/apollo-graphql-pagination-failed-with-limit However, using rendered props, i face the same issue whereby the offset variable cannot be changed (and is stuck to the initial offset variable) using fetchmore https://stackoverflow.com/questions/53446467/offset-variable-does-not-update-with-apollo-fetchmore it would be excellent if there is a fix to allow the rendered props method to work correctly. After all, i understand rendered props is the preferred approach going forward. Thank you

@abhiaiyer91 abhiaiyer91 removed their assignment Jan 29, 2019
@mnlbox
Copy link

mnlbox commented Feb 18, 2019

@hwillson It's still not working for me.
Any plan to fix this?

@FunkSoulNinja
Copy link

I had this problem and found a workaround for the issue.
To give you some context: I was using React and had a button to fetch more posts inside a Query component using cursor-based pagination.

Instead of calling the fetchMore function received from the Query component, I would:

  1. Let my component do it's job with my initial variables or whatever.
  2. Directly call the query method on the client object with different variables and save the response onto a variable named "res":

const res = await client.query({ query: getPosts, variables: { ...differentVariables })

  1. Use client.readQuery to read from the cache and save it to a variable:

const prev = client.readQuery({ query: getPosts, variables: { ...myVariables })

  1. Lastly, combine the items and write it directly to the cache like so:

const updatedData = { ...prev, posts: [...prev.posts, ...res.data.posts] } client.writeQuery({ query: getPosts, variables: { ...myVariables }, data: updatedData })

This worked flawlessly for me.

@JClackett
Copy link

Still having a similar issue:

  const nextPage = (skip: number, currentSearch = "") =>
    fetchMore({
      variables: { houseId, skip, search: currentSearch },
      updateQuery: (prev, { fetchMoreResult }) => {
        if (!fetchMoreResult || !prev.allCosts || !fetchMoreResult.allCosts)
          return prev
        return Object.assign({}, prev, {
          allCosts: {
            ...prev.allCosts,
            costs: [...prev.allCosts.costs, ...fetchMoreResult.allCosts.costs],
          },
        })
      },
    })

I call this function when reaching the bottom of the page.

It works perfectly when the search variable remains as "", when reaching the bottom the skip variable changes and new data is refetched and works great.

However when i change the search variable and refetch the original query, and then try and paginate, i get an "ObservableQuery with this id doesn't exist: 5" error, I had a look in the source code/added some logs and it seems that apollo is using the old queryId (from the query where the search variable was "") for the fetchMore. I can see the data being successfully fetched in the network with all the correct variables but saving it back to the cache seems to be where it's erroring, am i missing something with this implementation?

Note: I am using react-apollo-hooks, which i know isn't production ready or even provided by apollo-client right now, but from what i've seen it looks like its something to do with the fetchMore API

Note: @FunkSoulNinja solution works for me, however would be nice to be able to use the provided API for this kind of feature

@MarttiR
Copy link

MarttiR commented Mar 7, 2019

This is still an issue on 2.4.13. Could @hwillson consider reopening this?

The workaround by @FunkSoulNinja above works, but becomes convoluted – here's a more complete example of how it goes together:

import React from "react";
import { Query, withApollo } from "react-apollo";
import gql from "graphql-tag";

const QUERY = gql`
  query someQuery(
    $limit: Int
    $offset: Int
  ) {
    someField(
      limit: $limit
      offset: $offset
    ) {
      totalItems
      searchResults {
        ... on Something {
          field
        }
        ... on SomethingElse {
          otherField
        }
      }
    }
  }
`;

const SearchPage = props => {
  const { client } = props;

  const defaultVariables = {
    limit: 10,
    offset: 0,
  };

  return (
    <Query query={QUERY} variables={defaultVariables}>
      {({ loading, error, data, updateQuery }) => {
        const { someField } = data;
        const { searchResults, totalItems } = someField;
        return (
          <>
            <ResultList results={searchResults} total={totalItems} />
            <Button
              onClick={async () => {
                const nextVariables = Object.assign({}, defaultVariables, {
                  offset: searchResults.length,
                });
                const prevVariables = Object.assign({}, defaultVariables, {
                  offset: searchResults.length - defaultVariables.limit,
                });
                const [next, prev] = await Promise.all([
                  client.query({
                    query: QUERY,
                    variables: nextVariables,
                  }),
                  client.query({
                    query: QUERY,
                    variables: prevVariables,
                  }),
                ]);
                const updatedData = {
                  ...prev.data,
                  someField: {
                    ...prev.data.someField,
                    searchResults: [
                      ...prev.data.someField.searchResults,
                      ...next.data.someField.searchResults,
                    ],
                  },
                };
                updateQuery(
                  (_prev, { variables: prevVariables }) => updatedData
                );
                client.writeQuery({
                  query: QUERY,
                  variables: nextVariables,
                  data: updatedData,
                });
              }}
            >
              Load more
            </Button>
          </>
        );
      }}
    </Query>
  );
};

export default withApollo(SearchPage);

@FunkSoulNinja
Copy link

@MarttiR You could also use the client object from the Query component render prop function without having to use the withApollo HOC.

@kandasj
Copy link

kandasj commented Sep 11, 2019

@FunkSoulNinja can you post the full solution please. I'm having the same issue. struggling to put together your solution

@muuuuminn
Copy link

Still have the same issue(2.6.4)

@RyotaBannai
Copy link

RyotaBannai commented Jun 29, 2020

There is a workaround for this... or maybe the solution

  1. set fetchPolicy as default
  2. add @connection directive to the field in your query so that apollo can identify which to concat...

like below...

const GET_ITEMS = gql`
  query GetItems($skip: Int, $take: Int, $current: Int) {
    getItems(skip: $skip, take: $take, current: $current) @connection(key: "items") {
      id
      data
      type
      list {
        id
        name
      }
    }
  }
`;

let fetch_options = { skip: 0, take: 2, current: 0 };
export const Pagination: React.FC<Props> = () => {
  const { called, loading, data, fetchMore } = useQuery(GET_ITEMS, {
    variables: fetch_options,
    // fetchPolicy: "cache-and-network", //   Do not set 
  });
  if (called && loading) return <p>Loading ...</p>;
  if (!called) {
    return <div>Press button to fetch next chunk</div>;
  }
  return (
    <div>
      <Button
        onClick={(e: any) => {
          fetch_options = {
            ...fetch_options,
            current: fetch_options.current + 1,
          };
          fetchMore({
            variables: fetch_options,
            updateQuery: (
              previousQueryResult,
              { fetchMoreResult, variables }
            ) => {
              if (!fetchMoreResult) return previousQueryResult;
              return {
                getItems: previousQueryResult.getItems.concat(
                  fetchMoreResult.getItems
                ),
              };
            },
          });
        }}
      >
        Fetch Next
      </Button>
  );
};

@mnguyen96
Copy link

@RyotaBannai That didnt work for me :/

@kyle-west
Copy link

It's 2021 and I am having this issue too. My work around was to use a ref to keep track of the cursor

// wrap useQuery
export function usePaginatedQuery(query, options, updateQuery) {
  const cursorRef = useRef(0) // use a ref since updating it does not cause a re-render
  const results = useQuery(query, options)

  // handle pagination
  const getMore =
    (() =>
      results
        .fetchMore({
          variables: { cursor: ++cursorRef.current },
          updateQuery,
        })

  return {
    ...results,
    getMore,
  }
}

@jonasdumas
Copy link

+1

@salesfelipe
Copy link

salesfelipe commented Apr 7, 2021

+1

Using the HOC it works fine, the problem is trying to use the fetchMore from the Query component...

@salesfelipe
Copy link

+1

Using the HOC it works fine, the problem is trying to use the fetchMore from the Query component...

Actually, my problem was that I was passing the variables in the wrong format and It seems that since the variables were in the wrong format he was just reusing the old input...

@hassanpdn
Copy link

+1

@cristian-milea
Copy link

hi. using "apollo-client": "^2.6.8", and having the same problem
This is an issue from 4 years ago. Amazing :))

@JonasRothmann
Copy link

Issue still exists in "@apollo/client": "^3.5.0-beta.16"

@lukasz-swider
Copy link

Anyone found a reliable solution ?
@hwillson any intel?

@ViktorVojtek
Copy link

Check the variables object you are passing to fetchMore function. It could be the possible issue, you can somehow easily miss...

I've encounter this issue recently on @apollo/client >3.4.10. But there was no problem with the apollo...

I was passing a bad variables object to fetchMore function.
My initial variables was for example: { variables: { input: { first: 10, cursor: undefined } } }

But I was passing to fetchMore function the following variables object: { variables: { first: 10, cursor: "xyzCusro.string" } }

@lukasz-swider
Copy link

Check the variables object you are passing to fetchMore function. It could be the possible issue, you can somehow easily miss...

I've encounter this issue recently on @apollo/client >3.4.10. But there was no problem with the apollo...

I was passing a bad variables object to fetchMore function. My initial variables was for example: { variables: { input: { first: 10, cursor: undefined } } }

But I was passing to fetchMore function the following variables object: { variables: { first: 10, cursor: "xyzCusro.string" } }

Cursor based pagination is working fine, but offset based pagination is glitched...

@matinzd
Copy link

matinzd commented Jan 19, 2023

Issue still exists on 3.6.5. Tried changing fetch policies but none worked. Variables are not getting updated using fetchMore.

const loadMoreItems = async () => {
    if (data && data.getCollection.itemsNextPageToken) {
      fetchMore({
        variables: {
          id,
          itemsPageSize,
          itemsPageToken: data.getCollection.itemsNextPageToken,
        },
        updateQuery: (prevQueryResult, newQueryResult) => ({
          getCollection: {
            ...newQueryResult.fetchMoreResult.getCollection,
            items: uniqBy(
              prevQueryResult.getCollection.items.concat(
                newQueryResult.fetchMoreResult.getCollection.items,
              ),
              'id',
            ),
          },
        }),
      })
    }
  }

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

No branches or pull requests