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

Implement fetchMore API for pagination #144

Closed
chimon2000 opened this issue Dec 31, 2018 · 24 comments
Closed

Implement fetchMore API for pagination #144

chimon2000 opened this issue Dec 31, 2018 · 24 comments
Assignees
Labels
enhancement New feature or request next Feature or report is related to next
Projects
Milestone

Comments

@chimon2000
Copy link

Is your feature request related to a problem? Please describe.
Currently, there is not an elegant way to do pagination with a GraphQL query. Apollo supports this thru the fetchMore API that is part of the query result.

Describe the solution you'd like
In Apollo, the fetchMore function allows you to do a new GraphQL query and merge the result into the original result. You can override variables, and provide a callback function to update the previous query.

Here's a simple example in in JavaScript

fetchMore({
  variables: {
    offset: data.feed.length
  },
  updateQuery: (prev, { fetchMoreResult }) => {
    if (!fetchMoreResult) return prev;
    return Object.assign({}, prev, {
      feed: [...prev.feed, ...fetchMoreResult.feed]
    });
  }
});

Describe alternatives you've considered
Right now the best solution I can come up with is to implement this logic with a GraphQLConsumer that gives me access to the GraphQLClient. This isn't the worst, but it does require quite a bit of boilerplate code.

Additional context
https://www.apollographql.com/docs/react/features/pagination.html

@wwwdata
Copy link
Contributor

wwwdata commented Jan 17, 2019

Hi, we are currently evaluating this plugin and I also asked myself how to implement fetchMore. Actually it is quite simple to make a widget that extends the standard Query widget and provides this functionality like in the javascript apollo client. I have just extended the builder with the fetchMore callback.

I could also open a pull request into the next branch with this. But the question would be: Make this its own widget or integrate it into the base Query widget? For that we would need to permanently add the fetchMore parameter (maybe as optional one) into the builder function signature.

import 'package:flutter/material.dart';
import 'package:graphql_flutter/graphql_flutter.dart';

typedef void FetchMoreCallback(
  Map<String, dynamic> variables,
  MergeResults mergeResults,
);

typedef Map<String, dynamic> MergeResults(
  dynamic prev,
  dynamic moreResults,
);

typedef Widget FetchMoreBuilder(
  QueryResult result,
  FetchMoreCallback fetchMore,
);

class QueryFetchMore extends StatefulWidget {
  final QueryOptions options;
  final FetchMoreBuilder builder;

  const QueryFetchMore({Key key, this.options, this.builder}) : super(key: key);

  @override
  QueryFetchMoreState createState() {
    return new QueryFetchMoreState();
  }
}

class QueryFetchMoreState extends State<QueryFetchMore> {
  bool _init = false;
  GraphQLClient _client;
  QueryResult _currentResult = QueryResult(loading: true);

  @override
  void didChangeDependencies() {
    super.didChangeDependencies();
    _client = GraphQLProvider.of(context).value;
    _initialQuery();
  }

  void _initialQuery() async {
    if (_init == false) {
      _init = true;
      final QueryResult result = await _client.query(widget.options);
      setState(() {
        _currentResult = result;
      });
    }
  }

  void _fetchMore(
    Map<String, dynamic> variables,
    MergeResults mergeResults,
  ) async {
    setState(() {
      _currentResult = QueryResult(data: _currentResult.data, loading: true);
    });

    final QueryOptions nextOptions = QueryOptions(
      document: widget.options.document,
      variables: variables,
      fetchPolicy: widget.options.fetchPolicy,
      errorPolicy: widget.options.errorPolicy,
      pollInterval: widget.options.pollInterval,
      context: widget.options.context,
    );
    final QueryResult result = await _client.query(nextOptions);

    if (result.errors != null) {
      setState(() {
        _currentResult =
            QueryResult(data: _currentResult.data, errors: result.errors);
      });
      return;
    }

    final QueryResult mergedResult = QueryResult(
      data: mergeResults(_currentResult.data, result.data),
    );

    setState(() {
      _currentResult = mergedResult;
    });
  }

  @override
  Widget build(BuildContext context) {
    return widget.builder(_currentResult, _fetchMore);
  }
}

@olup
Copy link

olup commented Feb 16, 2019

This is a very important feature to provide out of the box. To me it should be integrated in the same Query widget. @wwwdata have you ended up submitting your PR ?

@HofmannZ HofmannZ added the enhancement New feature or request label Feb 16, 2019
@HofmannZ HofmannZ added this to To do in Open Scrum via automation Feb 16, 2019
@HofmannZ HofmannZ added this to the 1.0.0 milestone Feb 16, 2019
@HofmannZ HofmannZ added the next Feature or report is related to next label Feb 16, 2019
@wwwdata
Copy link
Contributor

wwwdata commented Feb 22, 2019

@olup not yet. Also noticed that I have to use the watcher to not break the "get stuff from cache first and then network" in my custom widget. So I have a slightly better version now.

How would an ideal API look like to query the next page? Should we add this as parameter for the builder? Or would it be better to add it as a function of QueryResult? I think there was another pull request/issue that wants to implement the "refetch" behaviour as a function of QueryResult right?

@HofmannZ what do you think would be a good API?

@olup
Copy link

olup commented Feb 22, 2019

@wwwdata good to hear ! At first, I thought mimicking the React implementation by having a method on QueryResult would be the best way, and I coded something in that prospect. However, seeing your implementation, I thought your way of proposing that on the widget level makes kind of better sense.

Also I was wondering about exposing the last variables given to the request, so that we can easily implement infinite scroll, for example, just adding the offset or the page on the last query variable, without even tracking it on the widget calling the query. There is probably other use cases too for that.

The last question for me would be if there should be a cache mechanism that could give back the all added query with latest variables - use case would be an infinite loading list that gets disposed at some point and would be called upon again later. It should show the all list. Right now, if i get it, cache works by keying each request with its variable.

@HofmannZ for thoughts on those questions.

@HofmannZ
Copy link
Member

@wwwdata I would say that adding it to the builder would be ideal. Though I do think the logic should be inside the QueryManger since it can reuse the query that is managed from there.

@olup Could you elaborate on why you think it should be implemented in the widget level?

@olup
Copy link

olup commented Feb 25, 2019

@HofmannZ I meant exposing it through the builder, rather than mimicking Apollo and having it inside the result. So I completely agree with you :

I would say that adding it to the builder would be ideal. Though I do think the logic should be inside the QueryManger since it can reuse the query that is managed from there.

Maybe other new tools could be exposed as such, so we could imagine :

return widget.builder(_currentResult, _fetchMore, _refresh, _lastVariables);

_refresh would be triggering again the initial query and update results and cache.

_lastVariables would be the last parameters, or maybe the complete query object, something along those lines.

@mainawycliffe
Copy link
Collaborator

@micimize Was this ever resolved?

Open Scrum automation moved this from To do to Done May 4, 2019
@mainawycliffe mainawycliffe reopened this May 4, 2019
Open Scrum automation moved this from Done to In progress May 4, 2019
@mainawycliffe mainawycliffe moved this from In progress to To do in Open Scrum May 4, 2019
@mainawycliffe mainawycliffe self-assigned this May 24, 2019
@mainawycliffe mainawycliffe moved this from To do to In progress in Open Scrum May 24, 2019
@micimize
Copy link
Collaborator

micimize commented Jun 1, 2019

@mainawycliffe looking at apollo's ObservableQuery.fetchMore, they:

  • fetch the query with updated variables once as a unique query
  • merge the results and variables based on the user-defined updateQuery
  • mutate the previous/base query and rebroadcast it in ObservableQuery.updateQuery
  • remove the individual fetchMore query

So essentially, future polling will just poll for the merged variables, which should describe the merged query according to the user

@mainawycliffe
Copy link
Collaborator

With that in mind, i can make it work now.

@Wisdom0063
Copy link

Please has this feature been added?

@mainawycliffe
Copy link
Collaborator

@Wisdom0063 been caught at work lately, but i will find sometime and wrap up this, this week.

@bearchit
Copy link

bearchit commented Jun 27, 2019

Any progress on this issue?

@mainawycliffe
Copy link
Collaborator

I have made some progress, will be available for testing either today or tomorrow.

@mainawycliffe
Copy link
Collaborator

I have a first iteration of the fetch more feature here, please try it and see if it solves your use case, and provide feedback here.

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Jun 28, 2019

@lucasavila00
Copy link
Contributor

lucasavila00 commented Jun 28, 2019

I have an idea. It's kind of like @wwwdata 's idea, but instead of keeping more data in the state we'd have a component called QueryFetchMore that gives a list of QueryResults instead of a QueryResult.
This way we still have refetch and cache support out of the box without having to make a breaking change.

I have a working example here

Altough it is not apollo-like it doesn't prevent another API to be present in the future, but right now it seems that it would be a lot of work to get the cache and other features to work with a stateful solution as was proposed.

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Jun 29, 2019

@lucasavila00 The reason i didn't take that approach is because we are trying to closely match this library APIs to those of react-apollo. You can see how react-apollo does it the docs here.

Here is an example from react-apollo:

const FEED_QUERY = gql`
  query Feed($type: FeedType!, $offset: Int, $limit: Int) {
    currentUser {
      login
    }
    feed(type: $type, offset: $offset, limit: $limit) {
      id
      # ...
    }
  }
`;

const FeedData = ({ match }) => (
  <Query
    query={FEED_QUERY}
    variables={{
      type: match.params.type.toUpperCase() || "TOP",
      offset: 0,
      limit: 10
    }}
    fetchPolicy="cache-and-network"
  >
    {({ data, fetchMore }) => (
      <Feed
        entries={data.feed || []}
        onLoadMore={() =>
          fetchMore({
            variables: {
              offset: data.feed.length
            },
            updateQuery: (prev, { fetchMoreResult }) => {
              if (!fetchMoreResult) return prev;
              return Object.assign({}, prev, {
                feed: [...prev.feed, ...fetchMoreResult.feed]
              });
            }
          })
        }
      />
    )}
  </Query>
);

As you can see the fetchMore method is exposed by the builder, which you can trigger anytime as i have done in the example i provided. And since we have to pass the fetchMore back to the builder, we have a breaking change.

The problem with multiple components, is to what end because the API will continue to grow to support more features, which until dart supports union type definitions, they will be breaking changes, we have a similar issue with refetch.

We can solve this by having a snapshot class for now that will contain query results, refetch and fetchmore call back, but it will still be a breaking change.

@lucasavila00
Copy link
Contributor

I know apollo and I really like their api.
The problem in doing this like they do is that they inject __typename into their queries and they have all the types cached and normalized. That way if you refetch someting that was used in FetchMore it will be transparently updated for you.
The current implementation of graphql_flutter does not inject __typename into the queries and does not cache and normalize every type to make refetches and caches updates transparents.

I plan to submit the API I proposed on #176 for direct cache access and it wouldn't work with the API you proposed.

I believe that my implementation isn't a breaking change because it's only a new class, it doesn't change any other file, but let me know if I'm wrong (I have never been a mantainer of something myself) and as a bonus it supports the current refetch implementation and the proposed direct cache access one.

And if, in the future, when all the types are being cached and normalized my implementation wouldn't stop the api surface you're proposing to be implemented.
I'd like to upstream what I have so that other people make sure it keeps working in the future, please excuse me if I'm being too much supportive of my idea.

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Jun 29, 2019

I don't mind you supporting your own idea, but IMO i don't think we should branch from apollo implementation unless we have to. I don't think this feature warrants a new FetchMore Widget on top of the current Query Widget. Your concerns about cache and normalization can be addressed, because under the hood, the two implementation do the same thing, and the key difference being how the FetchMore method is exposed for consumption. I am basically choosing to stash the fetchMore method under the observable query class, like apollo, but i started out with it in the same location as yours but in the Query Widget.

On top of that, i see you are now allowing the user to determine how the results are combined, or am i missing something.

@lucasavila00
Copy link
Contributor

The user gets back a List of QueryResults and do with it as they please.
The real problem right now is inject __typename into the queries but I don't know how hard is it to implement this feature.

@mainawycliffe
Copy link
Collaborator

mainawycliffe commented Jun 29, 2019

That's the purpose of the updateQuery method. The user passes the parts of the query that have change changed and an updateQuery method, which is a call back. Once the results are returned, the the call back is used to merge the results of the previous results with new results, getting a single query results. This is then added to the query stream triggering a rebuild. All decisions from how to run the query, to how the results are made by the developer, just like in the Apollo example I gave above, and the Apollo docs, I linked. The example I linked also shows how the feature works, everything happens transparently.

FetchMoreOptions opts = FetchMoreOptions(
  variables: {'cursor': pageInfo['endCursor']},
  updateQuery: (prev, res) { // callback for how the data will be merged, the developer determines how to merge the database based on expected response.
    final List<dynamic> repos = [
      // in this just combine the list of repos from previous and current
      ...prev['search']['nodes'] as List<dynamic>,
      ...res['search']['nodes'] as List<dynamic>
    ];
    
    // set it to the nodes of one and return  the new response
    res['search']['nodes'] = repos;

    return res;
  },
);

About injecting type name, I will make the changes some changes by tomorrow and see if I can get it to work. Am thinking of adding query ids to the fetch more requests just like Apollo does, I think that will solve the problem.

The other thing I noted with Apollo is that when a user submits a new documents, options of the previous queries are all ignored, in the end using fetch more options only.

@mainawycliffe
Copy link
Collaborator

This issue can now be closed, it is currently available on beta and will be released to stable soon. If you come across any new bugs, issues, please create a new issue.

Open Scrum automation moved this from In progress to Done Aug 8, 2019
@AkhilSajiCodzoc
Copy link

AkhilSajiCodzoc commented Oct 1, 2020

@mainawycliffe im implemented fetchmore as you doen here ..its wors and data updates on the array .problem is query widget rebuilds when i calls fetchmore .after updates we can see the item from first.can you give me the solution for this?

@pokapow
Copy link

pokapow commented Dec 25, 2020

I have got the same problem than @AkhilSajiCodzoc, when using @mainawycliffe way to fetchmore pages on infinite scroll, the request seems to execute 2 times or more, and don't seems to load only the page we want but query everything since first page, each time, for each pages. Is it normal ?

Is fetchmore API actualy work "out of the box" in V4 beta ?

Thank's for your work anyway and Merry Christmas !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request next Feature or report is related to next
Projects
No open projects
Open Scrum
  
Done
Development

No branches or pull requests