[On hold] Fetch more / Infinite Scroll

[rricard]

Adds a fetchMore method able to fetch new data and concatenate it into the store, performing, that way, an infinite scroll behavior!

TODO:

• Update CHANGELOG.md with your change
• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests and linter rules pass
• change concatPaths: Array<string> to paginationParameters: Array<string>, that way suppressing for special paths those parameters from the store ids. (ex: instead of ROOT_QUERY.allPosts({cursor: 'x', type: 'y'}), if paginationParameters: {'ROOT_QUERY.allPosts': ['cursor']} is set, it will only be ROOT_QUERY.allPosts({type: 'y'})
• Implement correct resolution of paths
• Squash!
• paginationParameters => quietArguments
• Backwards concat possible
• Add directive for pagination !
• Add a per-directive merge option
• Add a directive selection in the fetch more to avoid fetching more of everything
• fetch more can override query-level opts
• embed into the directive common merging use cases: append/prepend/sort
• Make sure is correctly wired (fetchMore() to the public API, paginationParameters everytime there is some store access & `fetchMore everytime we're doing a query)
• Do a proper system to handle directives (right now it's clearly hacky) => REFACTOR!
• So do MORE TESTS and/or integrate with existing tests to take paginationParameters in account.
• Directive removal before sending the query to the server! OR PR on the server to support the directive there (by adding it to the schema! Maybe somewhere in graphql-tools)
• Add it to the docs!

Basic usage example:

let latest_name = null;
const subscription = client.watchQuery({
  query: gql`
    all_people(latest_name: $latest_name, type: $type) @apolloFetchMore {
       name
    }
  `,
  quietArguments: ['latest_name'],
  variables: {
    type: 'Jedi',
    latest_name: null,
  },
}).subscribe({
  next(result) {
    console.log(result);
    // First value => {all_people: [{name: 'Luke SkyWalker'}]}
    // Second value => {all_people: [{name: 'Luke SkyWalker'}, {name: 'Obi-Wan Kenobi'}]}
    const new_latest_name = result.all_people[result.all_people.length -1];
    if(new_latest_name !== latest_name) {
      latest_name = new_latest_name;
      subscription.refetchMore({latest_name});
    }
  }
});

Advanced usage example:

let latest_name = null;
const subscription = client.watchQuery({
  query: gql`
    messages(cursor: $chatCur) @apolloFetchMore(name: "messages") {
      content
      replies @apolloFetchMore(name: "replies") {
         content
       }
    }
  `,
  quietArguments: ['cursor'],
  variables: {
    chatCur: null,
  },
  mergeResults: {
    messages: (oldArr, newArr) => [].concat(newArr, oldArr),
  }
}).subscribe({
  next(result) {
    console.log(result);
    setTimeout(() => {
      subscription.refetchMore({targetedFetchMoreDirectives: ['replies']});
    }, 60000);
    setTimeout(() => {
      subscription.refetchMore({targetedFetchMoreDirectives: ['messages']});
    }, 10000);
  }
});

Advanced usage example with directives only!:

let latest_name = null;
const subscription = client.watchQuery({
  query: gql`
    messages(cursor: $chatCur) @apolloFetchMore(name: "messages", quiet: "cursor", prepend: true) {
      content
      replies @apolloFetchMore(name: "replies") {
         content
       }
    }
  `,
  variables: {
    chatCur: null,
  },
}).subscribe({
  next(result) {
    console.log(result);
    setTimeout(() => {
      subscription.refetchMore({targetedFetchMoreDirectives: ['replies']});
    }, 60000);
    setTimeout(() => {
      subscription.refetchMore({targetedFetchMoreDirectives: ['messages']});
    }, 10000);
  }
});

[rricard]

Followup to #335

[stubailo]

Can you rebase on master, as well?

[rricard]

yep, let me fix a test first

[rricard]

I already rebased minutes ago ...

[rricard]

And ... all done! Ok, I leave you the repo as it is now, I won't be able to contribute this weekend on this part (need to send my macbook to support) but I'll be back on monday! I may write some doc on an another computer though ...

[stubailo]

OK @rricard are you OK if I just work on this and maybe merge it?

[stubailo]

Just really excited to deliver this feature to our app team and see if they can use it in Galaxy.

[rricard]

No problem! I started that work so I could use the feature so that's cool if you take it from here. If you don't understand something I did, ping me here or on slack.

[stubailo]

Well I've got about 6 hours today to try to finish this and #336, let's see if I can get it done! Days without meetings don't come by too often :]

[stubailo]

So I think I'm going to remove a few things, in particular the sorting options in the directive. I think I'd rather add less stuff for now, and add more options later if it turns out the minimal approach is not convenient.

[jbaxleyiii]

@stubailo do you have an ETA for this? I'd rather not hack together a solution for something I need to ship if this is close!

[stubailo]

I'd guess merge by Tuesday. I was planning on doing it Friday but didn't get it finished.

[rricard]

Let me know if you need some help!

[rricard]

Ok, so I bumped apollo's max Gzipped file to 36kB.

[stubailo]

FYI, rebased on master.

[rricard]

okay pulling the rebased version

[rricard]

@stubailo not a fan of your directive validation simplification. The goal of that was to be able to let the user integrate custom directives later... If we use this hardcoded validation (that I implemented at first as well) this will prevent easily inserting new custom and easily validable directives later.

[stubailo]

OK I have one concern right now: how does polling/refetch work with this pagination model?

• refetch: I think it's OK if refetch just fetches the original query, and ends up throwing away the paginated data completely. For the use cases I can imagine, refetch ends up basically "restarting" the pagination from the beginning.
• pollInterval: I feel like the whole point of pagination is to avoid loading all of the data at once, so it would be odd to use polling with it. The one use case I can think of is polling for new data in the list - let's say you have a comment thread and you want to display new comments as they come in. But I feel that this might be a pretty uncommon use case so maybe we can just disable the combination of fetchMore and polling.

[stubailo]

@rricard honestly I don't think it's a good idea to write a general validation system if we aren't going to use it right now. Who knows what kind of validation we will need in the future? Maybe we won't need any general validation, in which case this solution is good, or maybe we will need very complex validation with dependencies between arguments, in which case we can't predict what we will need.

[rricard]

pollinterval will work like a refetch like it does right now. I don't know if we can customize that in order to do paginated polling

[stubailo]

@rricard I feel like it would be weird if someone set up a pollInterval, then scrolled down/loaded more data, and then the data was thrown away because the polling interval was hit. I'd be in favor of throwing an error if polling and fetchMore are used in one query, what do you think?

[rricard]

for the validation system: I will soon have validation needs but fair enough, I'll do an another PR when the need will be imminent. Basically here is my thought process: if graphql-js allows to add custom directive validation, we should be able to do that (or let that happen) as well on the client. So if you don't want anything to do with validating custom directives, maybe just let unknown directives pass through (and let the server validate them)

[rricard]

pollInterval+pagination won't play well together I agree. I don't think they should. When you take control with a fetchMore, you now become responsible for loading stuff (and merge it) in the order you need it.

[stubailo]

Personally I think it would be fine to let through unknown directives. This, combined with custom network interfaces, could be an easy way to implement @defer, @stream, etc.

Do you think directive validation via static analysis (ESLint etc) is sufficient? Or maybe it can be something that is done only in a development build? I feel like it can get pretty complicated and it seems odd to do all of that work and load all of the logic at runtime.

pollInterval+pagination won't play well together I agree. I don't think they should. When you take control with a fetchMore, you now become responsible for loading stuff (and merge it) in the order you need it.

Cool, let's make it throw an error somehow.

[stubailo]

Let me know if you think this is reasonable, but my goal is to merge something that is as minimal and flexible as possible, then add additional options and built-in stuff once people have a chance to try it out. This is why I removed some of the things you had already implemented like special sorting options, prepend, etc, since I think they are covered by the mergeResults function.

[rricard]

Ok, @stubailo, it's done! Had to just go slower and structure things more correctly. Now the annotation is never sent to the server but is still usable from within the client (especially when writing the store. I managed to get that by transforming the query as immutably as I could.

[rricard]

Now we just need to figure out what we finally want:

• Do we keep quiet on the directive ? (I would say yes because it can give a per-field control if you are doing something really complex)
• Do we rename quietArguments and how if yes ? (I would say something like storeIgnoredArguments but not paginationArguments => too specific. But I also like quietArguments, I mean, it works!)

[stubailo]

Now the annotation is never sent to the server but is still usable from within the client (especially when writing the store.

Thanks for that - I think your solution is great, basically what I was trying to do as well.

Do we keep quiet on the directive ? (I would say yes because it can give a per-field control if you are doing something really complex)

Another way we could get per-field control is if we made the quietArguments option in JavaScript also take a dictionary of names, which would be more consistent with how mergeResults works now. That way, you can pass around these arguments as JavaScript variables rather than adding them to the query every time.

Do we rename quietArguments and how if yes ?

My main concern here is if someone new is reading the code, and sees "quietArguments", I think they would get pretty confused. I guess at that point you could go read the docs, but it seems like something even like "paginationArguments" or maybe even "ignoredArguments" or similar actually gives you an idea of what it's for.

[rricard]

Ok, I need to leave work so I may let you do that. I agree with what you said 👍

[stubailo]

Todos for me:

• Remove quiet on the directive
• Make quietArguments able to be a dictionary, like mergeResults
• Don't put mergeResults functions in the store, since then it won't be serializable
• Rename quietArguments to something more descriptive
• Add test for quietArguments being used as a dictionary

This is turning out to be a lot of work! But I think it will be worth it. We just need to make sure when we merge everything it's maintainable. But perhaps that can be done in a refactor for a later major version bump.

[stubailo]

Hey, so to be honest, I'm having a really really hard time reasoning about the possible implications of all of these changes, especially because there are a lot of things together. I think this PR kinda became too big for its own good.

I think this is mostly my fault, because I didn't put in effort at the beginning to encourage splitting this into smaller chunks of work -- I'll make sure it doesn't happen again in the future.

I'm going to try a different approach for this feature, which is to try to split this up into the smallest PRs possible, each with their own tests etc.

For example:

1. Passing through custom directives
2. Stripping apollo client-only directives
3. Getting access to apollo client-only directive information inside write/read to store functions
4. fetchMore with no options, and only append
5. fetchMore with custom behaviors, and only one per query
6. fetchMore with multiple apolloFetchMore directives per query

For each of those, it will be a lot easier for me to reason about whether we have good test coverage, if there are weird edge cases, etc.

Sorry for anyone who was relying on this feature getting merged this week - I'd rather take a step back now than merge something that I don't fully understand the implications of.

[stubailo]

Here's the first PR, to not throw on unknown directives: #372

[rricard]

@stubailo: we'll wait. But let me help you though, I'll try to break down that today.

[rricard]

@stubailo: I think that's a pretty great decision in the long run. Even in order to get this merged, I think that's a good idea since it will make the review easier therefore faster.

[rricard]

@stubailo: made a PR for stripping apollo directives