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

pollInterval not working with offset-based fetchMore pagination #1087

Closed
SachaG opened this issue Dec 24, 2016 · 34 comments
Closed

pollInterval not working with offset-based fetchMore pagination #1087

SachaG opened this issue Dec 24, 2016 · 34 comments

Comments

@SachaG
Copy link

SachaG commented Dec 24, 2016

I'm using fetchMore to implement offset-based pagination as suggested in the docs, and that works. But when I add pollInterval to refresh the data, it wipes out all the newly loaded data every time the interval triggers.

What am I doing wrong?

@SachaG
Copy link
Author

SachaG commented Dec 24, 2016

From reading the docs it seems like pollInterval would only work properly with cursor-based pagination, unless I'm overlooking something?

If so that's a pretty big caveat to leave out. The documentation should probably make this a lot clearer.

@helfer
Copy link
Contributor

helfer commented Dec 27, 2016

@SachaG yeah, it won't work, because the polling actually overwrites the result from the fetchMore (as you pointed out). This won't be any different with cursor-based pagination. In order to make polling and fetchMore work together, we'd need to add some logic that updates the variables of the base query so that when the polling happens, the new query contains all the data, including the one that fetchMore returned.

For now, my recommended workaround would be to use subscriptions for that update, if possible.

@SachaG
Copy link
Author

SachaG commented Dec 27, 2016

Oh ok, thanks for the added details. I assumed since you can get the server to return a new cursor value, that new value would be used for polling instead of the original cursor.

One of my biggest gripes with Meteor (and one reason for migrating to Apollo in the first place) was that it never offered a good solution for pagination, so I really hope Apollo won't have the same problem!

I'll look into subscriptions, although I was hoping to use polling to reduce server load for performance reasons. But that's another discussion I guess.

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

I was talking about this issue with @tmeasday and he suggested not using fetchMore at all, and instead managing the pagination variables as state in another component and passing them down to the HOC.

I haven't tried this yet, but it seems like maybe it could be a better solution for pagination than fetchMore? Should I expect this issue to be fixed (maybe by giving fetchMore a way to update the original query variables somehow), or should I instead go in that other direction? Any thoughts @helfer?

@tmeasday
Copy link
Contributor

tmeasday commented Jan 6, 2017

fetchMore is to general to be fixable via the mechanism you've described @helfer -- the second query can be absolutely anything so how can you update the variables to include it? I guess we could offer an API to let the developer do anything they want to the variables but that's making things pretty complicated.

Still it may be the only way to get the best performance.

@tmeasday
Copy link
Contributor

tmeasday commented Jan 6, 2017

A more declarative alternative would be to offer an API to specify a query to run + patch in when the variables change (so you call setVariables rather than fetchMore). I would advocate for this approach because it fits better with declarative APIs like React.

@stubailo
Copy link
Contributor

stubailo commented Jan 6, 2017

Could that be built on top of fetchMore?

@tmeasday
Copy link
Contributor

tmeasday commented Jan 6, 2017

Hmm, I guess, but it would be weird if fetchMore was to offer an API to update the variables.

But yeah, I think an API like this would be great:

obs = client.watchQuery({
  query: ...,
  options: ...,
  fetchMore: {
    query: (oldOptions, newOptions) => ...,
    variables: (oldOptions, newOptions) => ...
    updateQuery(/* as before */)
  }
})

// and instead of calling `obs.fetchMore`, you'd call
obs.setOptions(...) // or `setVariables`

Then setOptions (or setVariables) could just call the implied fetchMore and not re-query.

Rather than an imperative "fetch more now and this is how to do it", it's a declarative "if you need to fetch more, here's how to do it"

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

Just thinking out loud: if we didn't want to add complexity to Apollo's client APIs, could we do this on the server? In other words, if the query returned limit and offset arguments (we'd define these two as special property names), Apollo would know to use these for polling instead of the base query's original variables:

const MoreCommentsQuery = gql`
  query MoreComments($offset: Int, $limit: Int) {
    moreComments(offset: $offset, limit: $limit) {
      offset
      limit
      comments {
        author
        text
      }
    }
  }
`;

It would then be the resolver's job to return the correct new offset and limit to enable proper polling?

@stubailo
Copy link
Contributor

stubailo commented Jan 6, 2017

By the way, why use fetchMore in the first place here? Why not just change the offset and fire a new query? Since the polling will eliminate the benefits of incremental loading anyway.

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

@stubailo I believe that's the approach @tmeasday was suggesting. And I really don't know where I could've gotten this weird idea to use fetchMore from… ;)

@stubailo
Copy link
Contributor

stubailo commented Jan 6, 2017

Sure I guess I just don't consider this a case of pagination - since pagination is usually used to load only some data and not to reload everything. So IMO semantically pagination with polling doesn't really make sense

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

Maybe it's because I'm coming from Meteor but I don't see why it doesn't make sense. If you have a paginated forum homepage like the Meteor forums, and someone edits one of the post titles, wouldn't you want that title to be updated without waiting for a reload?

@stubailo
Copy link
Contributor

stubailo commented Jan 6, 2017

Sure, but in that case you don't get any benefit from using fetchMore to load the new page, since you're going to reload the whole thing right afterwards anyway. I guess pagination is a misleading term - it should be something like "loading partial new data" or similar.

@tmeasday
Copy link
Contributor

tmeasday commented Jan 6, 2017

Maybe there's a small benefit in that the new data loads faster?

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

I'm just trying to explain that I'm just using fetchMore because that's what's in the documentation and at no point is it written that it doesn't work with polling.

You can't expect users to understand in advance the pros and cons of every possible pattern, we have to trust the docs at some point… So it's just not super helpful to be told that I'm not supposed to be using fetchMore when that's the only option offered by the documentation.

@SachaG
Copy link
Author

SachaG commented Jan 6, 2017

By the way, another thought: what if polling happened at the store level (so for the entire store)? This way queries wouldn't need to handle it anymore, and could simply receive updated documents from the store?

@stubailo
Copy link
Contributor

stubailo commented Jan 6, 2017

Oh I'm not criticizing your approach, just arguing against adding a new feature for polling and pagination. I agree we should at least update the docs. At the end of the day it's up to Jonas anyway.

@SachaG
Copy link
Author

SachaG commented Jan 7, 2017

OK, so I got rid of fetchMore. I would suggest updating the documentation to add a section about "normal" pagination (i.e. state-based) before fetchMore is mentioned, and making it clear that fetchMore-based pagination has drawbacks.

And also removing statements like There are basically two ways of fetching paginated data: numbered pages, and cursors since we just found a third way ;)

@SachaG
Copy link
Author

SachaG commented Jan 9, 2017

Related question: what would be a good way to trigger a callback once my container is done loading more data?

@tmeasday
Copy link
Contributor

tmeasday commented Jan 9, 2017

Doesn't fetchMore return a promise? Or do you mean in the case where you change the variables declaratively?

Can't think of any great ways but componentWillReceiveProps on the child will work.

@SachaG
Copy link
Author

SachaG commented Jan 10, 2017

Yeah I meant in the declarative case. I'll look into componentWillReceiveProps but I assume it might be tricky to figure out exactly which prop change corresponds to the "done loading more" event.

@calebmer
Copy link
Contributor

I haven’t read the full issue, but this is a concept that I’ve been thinking about abstractly. Not sure if this has been mentioned, but I really think that we need a separate data structure for pagination separate from the data we initially requested. This may potentially be solved by client fields.

@helfer
Copy link
Contributor

helfer commented Feb 4, 2017

I think the answer here is that it doesn't make sense to use fetchMore and polling together at the same component. Apart from warning people when they do that, but not do anything else. In the future, we may implement a separate structure for connections, which could make polling work on paginated lists.

@calebmer
Copy link
Contributor

Closing because we don’t currently intend to make polling work with fetch more. The implementation would just be to difficult 😣

However, we recognize the use case! Hopefully we can introduce a better to implement pagination soon.

@smeijer
Copy link

smeijer commented Jul 27, 2017

A little late, but one case for the fetchMore + polling is on infinite lists.

Just changing one variable doesn't work, as the start of the list is than being removed. And polling is wished for to update information along the way.

It can for example happen that an item moves from "page 2" to "page 1" when sorting on a modifiedAt.

After reading this issue, I'm still unsure what the way to go would be here.

@smeijer
Copy link

smeijer commented Mar 8, 2018

Wouldn't this issue actually be solvable when a poll used an updateQuery similar to fetchMore? If updateQuery had a parameter that told us if it was triggered by a poll, or a fetchMore request, we could update the prevResult with fetchMoreResult / pollResult accordingly.

@kadikraman
Copy link

I have an infinite list that uses fetchMore, but needs to be polled for items (think twitter). So basically same problem as y'all. This is how I solved it:

My API is already implementing the cursor approach where I send the id of the latest item and it'll give me the next 10.

  1. added a param firstItemId to my api where instead of sending the next 10, it returns all newer items
  2. added a query, nested inside the original paginated query that just polls the list with firstItemId every 2 seconds
  3. use onCompleted to manually add the new items to the cache

So far it works. I can't quite decide if it's a clever solution or just a giant hack. Maybe it's both 😄

@smeijer 's suggestion would 💯 fix this problem. Hope it gets implemented some time!

@maitriyogin
Copy link

maitriyogin commented Jun 7, 2019

Unfortunately I don't have the ability to query for all changed items in a list. Instead I'd need to increment the limit of my polling query based upon the scroll and subsequent calling of fetchMore. This could amount to a huge list which I'd need to merge into the cache... doable but kind of intensive.
Has any one else had luck with this kind of approach?
/Stephen.

@EJohnF
Copy link

EJohnF commented Aug 1, 2019

I've solved it by:

  1. save page number in outer component
  2. set partialRefetch and returnPartialData to true
  3. return Loading component only when loading and data is empty
         <Query
            query={DATA_SUBSCRIPTION}
            variables={{
              limit,
              offset: 0,
              orderBy: dataSort ? { [dataSort.property]: dataSort.directImport } : {},
              where: filters,
            }}
            pollInterval={400}
            partialRefetch
            returnPartialData
          >
            {({
              loading, error, data
            }) => {
              if (loading && !(data && data.contract_contract && data.contract_contract.length > 0))
                return <p>Loading...</p>;
<..>
           <InfiniteScroll
                    dataLength={camelData.length}
                    next={updatePage} // update limit variable in outer component
                    height={350}
                    hasMore={limit === camelData.length}
<...>

As result I have smooth infinity scrolling with polling for keeping info updated

@smeijer
Copy link

smeijer commented Sep 30, 2019

@helfer, is there a reason why we couldn't trigger the updateQuery from the poller, as I suggested above #1087 (comment)?

Even now we have hooks, this pattern keeps frustrating me. I just need a paginated list, in combination with a poll to refresh the "current view".

@adamsoffer
Copy link

adamsoffer commented Oct 19, 2019

@hwillson Any thoughts on @smeijer suggestion? Polling + paginating infinite lists seems like a common enough use case where it's worth exploring.

@in19farkt
Copy link

My solution for this:

const defaultPerPage = 10;
const poolInterval = 1000;

function SomeComponent() {
  const [currentPage, setPage] = useState(0);
  const [perPage, setPerPage] = useState(defaultPerPage);
  const [refetchTrigger, setRefetchTrigger] = useState(0);

  React.useEffect(() => {
    const interval = setInterval(() => setRefetchTrigger(Math.random()), poolInterval);
    return () => clearInterval(interval);
  }, []);

  const queryResult = useQuery(SOME_QUERY, {
    variables: { first: defaultPerPage, skip: 0 },
  });

  React.useEffect(() => {
    queryResult.fetchMore({
      variables: {
        first: perPage,
        skip: currentPage * perPage,
      },
      updateQuery: (prev, { fetchMoreResult }) => {
        return fetchMoreResult || prev;
      },
    });
  }, [queryResult.fetchMore, currentPage, perPage, refetchTrigger]);

  return (
    <div>
      <button onClick={() => setPage(currentPage + 1)}>Next</button>
      <pre>{JSON.stringify(queryResult.data, null, 2)}</pre>
    </div>
  );
}

@brayhoward
Copy link

brayhoward commented Aug 21, 2020

Workaround

  // Poll interval that works with pagination
  useEffect(() => {
    const intervalId = setInterval(() => {
      const total =
        (queryResult.data?.countSessions.edges.length || 0) +
        queryResult.variables.first!;

      queryResult?.refetch({
        ...queryResult.variables,
        first: total
      });
    }, 15_000);

    return () => clearInterval(intervalId);
    // eslint-disable-next-line
  }, [
    ...Object.values(queryResult.variables).flat(),
    queryResult.data?.countSessions.pageInfo.endCursor
  ]);

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests