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

[WIP] Optimistic UI #336

Merged
merged 28 commits into from
Jul 12, 2016
Merged

[WIP] Optimistic UI #336

merged 28 commits into from
Jul 12, 2016

Conversation

davidwoody
Copy link
Contributor

@davidwoody davidwoody commented Jun 29, 2016

TODO:

  • Use merge instead of assign to combine optimistic data
  • Optimistic reducer should use the exact same logic for data as a real mutation result
  • Update CHANGELOG.md with your change
  • Rebase your changes on master so that they can be merged easily
  • Make sure all tests and linter rules pass
  • Make sure all of the significant new logic is covered by tests

closes #287

@davidwoody davidwoody mentioned this pull request Jun 29, 2016
4 tasks
@abhiaiyer91
Copy link
Contributor

How does Optimistic Data get reset or trashed? I might have missed that in the PR?

@stubailo
Copy link
Contributor

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.

@abhiaiyer91
Copy link
Contributor

Bump on my question @davidwoody

@abhiaiyer91
Copy link
Contributor

What is the lifecycle of optimistic state?

@davidwoody
Copy link
Contributor Author

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.

@davidwoody
Copy link
Contributor Author

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.

@stubailo
Copy link
Contributor

stubailo commented Jul 1, 2016

So I just wait to throw away the optimistic results until all outstanding sever mutations resolve.

This seems like a good approach for now.

@stubailo
Copy link
Contributor

stubailo commented Jul 1, 2016

Hey, heads up @davidwoody I rebased on master and force pushed

@stubailo stubailo mentioned this pull request Jul 1, 2016
19 tasks
@davidwoody
Copy link
Contributor Author

@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

@Slava
Copy link
Contributor

Slava commented Jul 6, 2016

Plan for new tests for this functionality:

  • reuse the mutations tests structure for different types, that the result is optimistically reflected synchronously:
    • correctly integrates a basic object at the end
    • deletes object from array and store
    • runs the custom reducer
    • deletes an object from array but not store
  • test error handling
    • one optimistic update in a stack
    • multiple in a stack, one fails (in the beginning or middle)
  • two mutations running concurrently and returning in some order. every intermediate state is validated

@Slava
Copy link
Contributor

Slava commented Jul 8, 2016

Added some tests that demonstrate the problem of not passing the previous data state (with optimistic updates so far).

@Slava Slava force-pushed the optimistic-response branch 3 times, most recently from 8976186 to 7e72e8e Compare July 12, 2016 00:16
@Slava
Copy link
Contributor

Slava commented Jul 12, 2016

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)
Copy link
Contributor

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.

@stubailo
Copy link
Contributor

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.

@stubailo stubailo merged commit d5992f1 into master Jul 12, 2016
@zol zol removed the in progress label Jul 12, 2016
@Slava
Copy link
Contributor

Slava commented Jul 12, 2016

Yay we merged! Thanks @davidwoody for the good work on this feature.

@davidwoody
Copy link
Contributor Author

Woo hoo! Thanks @Slava for the great suggestions and work on all the tests!

@stubailo
Copy link
Contributor

Great work everyone!

@twelvearrays
Copy link

twelvearrays commented Jul 16, 2016

This is very exciting! Are there any examples available of using optimisticResponse with a client mutation?

This end of this post helped!
#398

@Slava
Copy link
Contributor

Slava commented Jul 17, 2016

@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!

@twelvearrays
Copy link

@Slava Thanks! Looking forward to it. Keep up the great work.

@leebenson
Copy link

leebenson commented Nov 2, 2016

Awesome work. A couple of Qs:

1. What's the usage for optimisticResponse on a mutation that has fragments?

I have a mutation in this format:

mutation destroySession {
  destroySession {
    ...Session
    ...Errors
  }
}

...Session has an id field I want to update.

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 optimisticResponse if multiple types returned an id, for example?

2. What's the optimisticResponse way to update a ObservableQuery subscription?

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.

@helfer
Copy link
Contributor

helfer commented Nov 2, 2016

  1. The optimistic response should basically be what you expect the server to return if everything works out as expected. That also means you have to predict what fragments will apply. It could be that there's a bug in the logic that checks that the shape of the optimisticResponse fits the query, but then that would probably throw an error on the actual response as well.
    (If I understand correctly, you're saying that using an optimisticResponse that matches the Error fragment doesn't work? If so, that might be a bug.)
  2. optimistic updates should affect all queries, and their subscriptions should trigger if the optimistic update affected their data. If that's not the case here, it could be a bug, or it could be that it's not actually mapped to the same place in the store. It would be great to have a minimal reproduction or a failing test case to figure out what exactly is happening.

@leebenson
Copy link

Thanks @helfer. I'll try to reproduce with a minimal test, and post the results here.

@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.

Optimistic updates
8 participants