-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
[WIP] Optimistic UI #336
[WIP] Optimistic UI #336
Conversation
How does Optimistic Data get reset or trashed? I might have missed that in the PR? |
We should also make sure that the optimistic result is applied using the merged real and optimistic data, so that you can apply multiple optimistic updates on top of each other. |
Bump on my question @davidwoody |
What is the lifecycle of optimistic state? |
Does the logic in the optimistic reducer make sense to you? Basically on init it simulates the result action and on the last result action it throws away the optimistic cache. It waits until the last outstanding result action to avoid the flickering issue I was explaining in Slack the other day. |
I wasn't sure how best to "throw away" the one optimistic result after the server result became available if there was still another optimistic result applied to the cache and waiting for the server responses. So I just wait to throw away the optimistic results until all outstanding sever mutations resolve. |
This seems like a good approach for now. |
6f50882
to
a1f1da9
Compare
Hey, heads up @davidwoody I rebased on master and force pushed |
@stubailo Got it. Also, it looks like the travis builds are failing because the gzipped size of index.min.js exceeds 34kb... I'm guessing we can just increase that from 34kb to 36kb? https://travis-ci.org/apollostack/apollo-client/jobs/141736810#L1343 |
Plan for new tests for this functionality:
|
Added some tests that demonstrate the problem of not passing the previous data state (with optimistic updates so far). |
8976186
to
7e72e8e
Compare
Completed the test suite planned by me and @stubailo awhile ago. |
@@ -44,6 +44,8 @@ Expect active development and potentially significant breaking changes in the `0 | |||
- Deprecate `apollo-client/gql` for `graphql-tag` and show a meaningful warning when importing | |||
`apollo-client/gql` | |||
|
|||
- Allow `client.mutate` to accept an `optimisticResponse` argument to update the cache immediately, then after the server responds replace the `optimisticResponse` with the real response. [Issue #287](https://github.com/apollostack/apollo-client/issues/287) [PR #336](https://github.com/apollostack/apollo-client/pull/336) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not in the right location, should be moved to the top.
Can we add a test that makes sure that the optimistic patch is small, and doesn't include objects that weren't changed? That seems like one thing that would be easy to break in future changes. |
Migrating changes over from PR #321
Still unable to get merge to be imported properly using the require('lodash.merge'); syntax
…ic updates (+test)
389080e
to
ea1b746
Compare
Yay we merged! Thanks @davidwoody for the good work on this feature. |
Woo hoo! Thanks @Slava for the great suggestions and work on all the tests! |
Great work everyone! |
This is very exciting! Are there any examples available of using optimisticResponse with a client mutation? This end of this post helped! |
@twelvearrays we are still in the process of designing a good API and ironing out the details of what is required to be passed and what is not. Once that's done better docs should come along! |
@Slava Thanks! Looking forward to it. Keep up the great work. |
Awesome work. A couple of Qs: 1. What's the usage for I have a mutation in this format: mutation destroySession {
destroySession {
...Session
...Errors
}
}
This is the only format that doesn't generate an error: const destroySessionMutation = {
mutation: destroySessionMutationGQL,
fragments: sessionFragment,
refetchQueries: [
'getSession',
],
optimisticResponse: {
destroySession: {
id: null, // <--- this is on `Session` - but that's only relevant when a `Session` is returned
},
},
}; How would I design 2. What's the I'm using Apollo alongside MobX, and using observable queries to update global MobX stores. A typical MobX store function that connects with Apollo looks like this: getSession = () => {
if (this.getSessionWatched) {
return this.getSessionWatched.refetch();
}
this.getSessionWatched = this.client().watchQuery({ ...sessionQuery });
return new Promise(resolve => {
this.getSessionWatched.subscribe({
next: res => {
resolve(res);
this.data.set('session.id', (res.data.session && res.data.session.id) || null);
},
});
});
} I expected watched queries would update in the interim whilst returning data, but it doesn't seem to invoke the subscription. What's the best way to connect the two? Thanks in advance. |
|
Thanks @helfer. I'll try to reproduce with a minimal test, and post the results here. |
TODO:
merge
instead ofassign
to combine optimistic datadata
as a real mutation resultcloses #287