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

Refetch promise #178

Merged
merged 8 commits into from
May 17, 2016
Merged

Refetch promise #178

merged 8 commits into from
May 17, 2016

Conversation

delianides
Copy link
Contributor

@delianides delianides commented May 5, 2016

This is for a change to the QueryManager on refetch to return a Promise instead of void. By returning a promise you eliminate the need to wrap any requests for updating data. So instead of:

refreshFeed = (resolve, reject) => {
  this.props.data.refetch();
  resolve();
}

<ReactPullToRefresh
  onRefresh={ this.refreshFeed }>
</ReactPullToRefresh>

You can call refetch directly from the Compontent:

<ReactPullToRefresh
  onRefresh={ this.props.data.refetch }
</ReactPullToRefresh>

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

@apollo-cla
Copy link

@delianides: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

@@ -12,6 +12,7 @@ Expect active development and potentially significant breaking changes in the `0
- Remove all dependencies on the `graphql` parser module at runtime, except for the `gql` template literal tag, so that queries can be pre-parsed in production to save on parsing overhead.
- Add everything that isn't a compiled file to `npmignore`. [PR #165](https://github.com/apollostack/apollo-client/pull/165)
- Move importable modules to root. [PR #169](https://github.com/apollostack/apollo-client/pull/169)
- Queries on refetch return promises.
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be in a new section at the top, since it will be in 0.3.2 or later.

@stubailo
Copy link
Contributor

stubailo commented May 5, 2016

Welcome @delianides!

@delianides
Copy link
Contributor Author

@stubailo Thanks! Everything should be updated.

@@ -341,13 +341,17 @@ export class QueryManager {
queryId,
requestId,
});

return result;
Copy link
Contributor

Choose a reason for hiding this comment

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

This won't be the correct result if forceFetch is set to false, because the network response from the server might only contain partial data. This should actually read the data from the store, using something similar to what watchQuery is doing. Going to look at this in more depth in a bit.

@johnthepink
Copy link
Contributor

Hey @stubailo I updated this:

  1. rebased it
  2. moved the client results above the return statements
  3. return data from the store, instead of just what comes back over the network
  4. prevent multiple errors from being thrown

@stubailo
Copy link
Contributor

Taking a look!

@@ -337,26 +350,43 @@ export class QueryManager {
queryId,
requestId,
});

return result;
}).then((result: GraphQLResult) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This argument isn't used anywhere, is it?

Copy link
Contributor

Choose a reason for hiding this comment

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

@stubailo you're right. updated.

@stubailo
Copy link
Contributor

@johnthepink looks good, just a few small comments!

@johnthepink
Copy link
Contributor

@stubailo updated except for the try/catch comment. Is there a better way to prevent the multiple errors?

@johnthepink
Copy link
Contributor

...and rebased to bring in new namespaced actions

@stubailo
Copy link
Contributor

@johnthepink OK, I think we can merge this now, we can handle the edge cases better later!

@stubailo
Copy link
Contributor

Might need to do another rebase, lol.

@johnthepink
Copy link
Contributor

@stubailo on it haha

@johnthepink
Copy link
Contributor

@stubailo and done

@stubailo stubailo merged commit a12faed into apollographql:master May 17, 2016
@stubailo
Copy link
Contributor

Published in 0.3.9!

jbaxleyiii pushed a commit that referenced this pull request Oct 18, 2017
@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.

5 participants