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

Initial implementation of fetchMore #472

Merged
merged 13 commits into from
Aug 1, 2016
Merged

Initial implementation of fetchMore #472

merged 13 commits into from
Aug 1, 2016

Conversation

Slava
Copy link
Contributor

@Slava Slava commented Jul 26, 2016

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

Design:

  • fetchMore method on an observable query
  • allows to fetch the same query, or a new query with any amended variables, fragments, etc
  • the results of the fetchMore query always are fetched from the server
  • the reducer function passed into fetchMore is responsible for merging the new result into the existing query results

@@ -16,8 +16,13 @@ import {

import assign = require('lodash.assign');

export interface FetchMoreOptions {
updateQuery: (previousQueryResult: any, options: any) => any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think options should be "any" here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, I should change these to have the precise fields.

@rricard
Copy link
Contributor

rricard commented Jul 27, 2016

If you need any help, just ask me @Slava, even if it's just writing tests!

@Slava
Copy link
Contributor Author

Slava commented Jul 27, 2016

The initial implementation completed. Have two tests covering all base cases. Still need docs, and some things.

@stubailo might want to look at the following "problem" in the API:

The reducer function receives two arguments: previousResult and the options with the fetchMoreResult.

  • Since previousResult is coming from the store, currently it looks like a plain result of the query not wrapped into { data: { ... }, errors: [] }.
  • On the other hand, we pass the query result from fetchMore options, directly into the reducer through fetchMoreResult and it is wrapped into { data: { ... }, errors: [] }.
  • Working with this API, the application developers are forced to face this inconsistency.
  • The same is true for mutations, the mutationResult is wrapped, the previous query result is not

The ways we can improve it include:

  • never pass wrapped values (need to think more if any error handling is damaged by such change)
  • always pass wrapped values (annoying to work with)

Either way, it will be a breaking change to mutations.

Resolution: Keep it as is for now. We can make another breaking change later if needed. The reasoning is to keep the wrapped objects, in case there is a different way of handling a query with errors. (cc @stubailo)

@Poincare Poincare mentioned this pull request Jul 30, 2016
4 tasks
@rricard
Copy link
Contributor

rricard commented Aug 1, 2016

Hey @Slava ! Any progress on this ? Do you need any help ?

@Slava Slava force-pushed the fetch-more-slava branch from 5c8ae58 to fc5d999 Compare August 1, 2016 18:51
@Slava
Copy link
Contributor Author

Slava commented Aug 1, 2016

@rricard Hi! No, I think this implementation is complete. We are working on merging it right now.

@stubailo stubailo merged commit 5649484 into master Aug 1, 2016
@rricard
Copy link
Contributor

rricard commented Aug 2, 2016

ok, I'll try that right away and give you feedback since the react-apollo PR seems to be there too!

@stubailo stubailo deleted the fetch-more-slava branch September 20, 2016 03:42
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants