Mutate Promise - Wait for query to be done

[appjitsu]

So let's say that I have this react app and there is a form with a submit button, which has the following click handler. What I would expect to happen is that, when the promise resolves the mutate command, the refetchQueries portion would already have occurred. But, that is not the case at all. What actually happens is that the refetchQueries and the router redirect happen at the same time. The .then(...) happens directly after I call mutate.

So how do I handle a situation like this?

import { graphql } from 'react-apollo';

...

onSubmit({ email, password }) {
    this.props.mutate({
      variables: { email, password },
      refetchQueries: [{ query }],
    })
    .then(() => { router.push('/dashboard') })
    .catch((res) => {
      const errors = res.graphQLErrors.map(error => error.message);
      this.setState({
        errors,
      });
    });
  }

...

export default graphql(mutation)(LoginForm);

[jzimmek]

I run into the same problem. Digged a bit into the apollo source:

apollo-client/src/core/QueryManager.ts

Lines 326 to 340 in 15c57fc

	if (typeof refetchQueries[0] === 'string') {
	(refetchQueries as string[]).forEach((name) => { this.refetchQueryByName(name); });
	} else {
	(refetchQueries as PureQueryOptions[]).forEach( pureQuery => {
	this.query({
	query: pureQuery.query,
	variables: pureQuery.variables,
	fetchPolicy: 'network-only',
	});
	});
	}
	delete this.queryDocuments[mutationId];
	resolve(<ApolloQueryResult<T>>result);

The query() invocations return promises but are simply not then-chained with the mutations resolve function.

Is this behavior a bug or by design? Will try to come up with a PR if this is a bug.

In the meantime I build my own small workaround to refetchQueries after a mutation. Maybe this helps anyone, too:

export default async (apolloClient) => {
  await Promise.all(Object.values(apolloClient.queryManager.observableQueries).map(({observableQuery}) => observableQuery.refetch(observableQuery.variables)))
}

[helfer]

@appjitsu @jzimmek just so I understand correctly, you would like the then to be called only after both the mutation and the subsequent refetches have completed?

It would be a breaking change, but I actually think it would be quite reasonable. @jzimmek would you like to make a PR for us to consider? If there's some code, we can see more clearly what the solution would look like 🙂

[appjitsu]

Yes, exactly. Wait for the mutations, etc to finish. Maybe make it so we need to pass some sort of flag for that to happen so it doesn't break existing expectations. I'm swamped at the moment, so I can't do a PR.

…

On May 2, 2017, at 1:33 PM, Jonas Helfer ***@***.***> wrote: @appjitsu @jzimmek just so I understand correctly, you would like the then to be called only after both the mutation and the subsequent refetches have completed? It would be a breaking change, but I actually think it would be quite reasonable. @jzimmek would you like to make a PR for us to consider? If there's some code, we can see more clearly what the solution would look like 🙂 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[ryannealmes]

I think I am running into a similar problem as well. It seems in my case the mutation and the refetches are happening at the same time. This causes the data being returned to not be the newly mutated data.

My mutation config

const config = {
  props: ({ mutate }) => ({
    makePayment: ({ registrationId, amount, currency, source, personCode, presentationCode }) => {
      let variables = { registrationId, amount, currency, source, personCode, presentationCode };
      return mutate({
        variables,
        refetchQueries: ['FindOutstandingPaymentsForPerson', 'FindPaymentHistory'],
      });
    },
  }),
};

Then in my component I call this.props.refetch()

this.props.makePayment({
      registrationId: this.props.registrationId,
      personCode: this.props.personCode,
      presentationCode: this.props.presentationCode,
      amount: this.props.invoicedAmountDue,
      currency: this.props.currency,
      source: token.id
    })
      .then(() => {
        setTimeout(() => { this.props.refetch(); }, 5000)
      })

As you can see the only way I can get this working is to add in a timeout so the refetch is delayed.

[jcheroske]

This situation is a common use case. Furthermore, having these sequences of events coupled to the UI (mutate -> change route -> send notification) is not always the best design. These flows occupy a kind of nebulous space between the UI and the business logic. My current thinking is that I'm going to place the mutate function in an action, along with its variables, and pass that into a side-effect (thunk, saga, epic, logic, etc). Then, in my side effect, I'd like to do stuff like:

// pseudo code
await mutate(variables)
await changeRoute(someURL)
await sendNotification('The thing happened')

This seems cleaner than having the flow embedded in the components. But, this all breaks down if the mutate promise is essentially meaningless!

[helfer]

If you care about knowing exactly when stuff has refetched, you can always fetch/refetch manually in the mutate().then ... function. That way you have complete control over what code runs when. You'll know when the mutation has completed, and you'll know when each of the refetches is done.

[jcheroske]

@helfer, can you provide an example of what you're talking about? I'm very new to Apollo.

[oopserv]

@jcheroske I believe @helfer is referring to something along these lines...

onSubmit ({ email, password }) {
    this.props.mutate({
      variables: { email, password }
    }).then(() => this.props.data.refetch())
  }

[oopserv]

Another good way to get around this is to use componentWillUpdate lifecycle method. You can check your props.data and compare if the 'old' user prop was logged out or didnt exist with the incoming props and see if they are now logged in or exist....

[helfer]

Yeah, @oopserv is right @jcheroske. What I'm proposing is basically this:

this.props.mutate({ ... }).then( () => {
  return refetchBlah(); // refetch blah can be this.props.refetch or client.query() ... or whatever.
})
.then( () => {
  // run some stuff at the very end.
});

[jcheroske]

But what about the issue @ryannealmes raised? Might the refetch run before the mutation has finished? I must be missing something...

[stubailo]

In the above code the refetch is guaranteed to happen after the mutation returns.

[stale]

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

[anhldbk]

So is there any solution for this issue?

[oopserv]

@anhldbk I handle this situation with componentWillUpdate lifecycle method. For example,

componentWillUpdate (nextProps) {
    if (!this.props.data.user && nextProps.data.user) {
      this.props.history.push('/userProfile')
    }
  }

[stale]

This issue has been automatically marked as stale becuase it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions to Apollo Client!

[stale]

This issue has been automatically closed because it has not had recent activity after being marked as stale. If you belive this issue is still a problem or should be reopened, please reopen it! Thank you for your contributions to Apollo Client!

[dbenchi]

IMO, this ticket should be reopen. I do not need to hack arround componentWillUpdate and leverage the nextProps so as to guarantee that the mutation along with the refetchQueries are all setteled.
I really want to know what the Apollo team thinks about this?
Thanks a lot

[flowirtz]

@abenchi agree with you - commenting to bump.

[lukewlms]

I think this needs attention as a high priority.

As someone who started using JS 1.5 years ago in the age of promises, I always expect the action to finish first when awaited. Axios, for example, will only resolve the promise after the full query is finished.

Having await apolloClient.mutate(...) resolve BEFORE the mutation + query fully resolves creates non-obvious bugs because the JS way is to always, fully finish meaningful actions before resolving.

Our use case: need the mutation to finish before popping to another screen with the new values confirmed as saved to server and fetched.

This would be a breaking change but I think it is pretty important. The old way could be deprecated and put behind a flag in the apolloClient constructor if desired.

[bermanboris]

Any updates on this issue? I think current behaviour is very confusing.

[mvaivre]

+1, it's definitely a confusing behaviour.

[n-hare]

+1

[francisngo]

+1

[Vitiell0]

@helfer have you guys thought about this anymore?

[shafqatevo]

+1

[jzimmek]

this issue has been created almost a year ago and i was heavily affected by it at that time. after switching projects in between i am now back into a new apollo based project. i am still running into the same issue. last time i had no time to really address it, then switched over to non-apollo based projects and forgot about it. this time i really wanna fix it.

i appreciate all the explanations and possible solutions (workarounds) for this. but for me a resolved promise should be 100% complete before being resolved - everything else is imho just confusing.

i think the problem can be addressed with minimal changes and i prepared a pull request for discussion: #3169

i have not yet created any dedicated tests for this change, but all existing tests pass.

before putting more time on this issue / pullrequest i want to ask upfront if:

• this issue is something apollo team would generally accept as pull request or not (keep behavior as is)
• it is fine to add async/await to the code-base (found those only in the unit tests)
• this should be put behind some flag to not make it a breaking change
• who this kind of change is being tested best (without introducing even more changes just for testing sake)

@helfer would love to hear some feedback from you. maybe you even have first thoughts by looking into the referenced pull request.

[findli]

+1

[alexandprivate]

Hello there @appjitsu, I'm facing kinda the same problem here ... my case I want to update the state in both cases, the the response is ok and on error after mutation, I notice you are doing this:

.catch((res) => {
      const errors = res.graphQLErrors.map(error => error.message);
      this.setState({
        errors,
      });
    });

Well that does not work for me it said that this.setState is not a function... I am doing something like this after the mutation:

.then( () => {
                    this.setState({
                        notifType:'success',
                        notifMessage:'Done!'
                    })
                }).catch( res => {
                    this.setState({
                        notifType:'fail;',
                        notifMessage:'Error'
                    })
                })

Any idea ? Thanks in advanced.

[DanielLaberge]

I'm sorry to be so blunt but this situation is unacceptable; We need an official word from the team ASAP.

We're currently trying @jzimmek's contribution but need to know whether this will be eventually merged or not. If not, we need to know why and what else can be done. We're willing to contribute but can't do so without official direction from the core team.

@hwillson Could you please weigh in on this, or assign someone to do so?

[hwillson]

Thanks for the ping here @DanielLaberge - and not too worry, bluntness is okay!

The reason why this hasn’t received greater attention, is that the current behaviour is actually by design. mutate refetchQueries was intended to be used as an async side effect. Waiting for the refetched queries to complete before resolving the mutation was not part of the original spec. The idea being that if anyone did want to wait they could always handle things manually, like #1618 (comment) mentions. So something like:

...
this.props.mutate({ ... }).then(() => {
  // Mutation has completed, so refetch.
  // `refetchBlah` can use `this.props.refetch` or 
  // `client.query()` or whatever.
  return refetchBlah(); 
}).then(() => {
  // Refetching has completed; continue on ...
});
...

The above being said, I think it’s safe to say this is a pain point for many. We’re planning on completely re-visiting how refetchQueries works for Apollo Client 3.0, but we’re not there yet. Given this, I’ll make getting a slight variation of #3169 merged in a priority, for Monday. I say slight variation as I’m a bit concerned about that PR not being backwards compatible. Making mutate all of a sudden wait for refetchQueries to complete could impact existing applications. I think that if we’re going to roll this functionality out sooner than later, then we’re going to have to enable/disable it via a config option. Something like awaitRefetchQueries , that is false by default. I’ll put some more thought into this, but regardless I should have something ready tomorrow.

Thanks for your patience everyone!

[DanielLaberge]

Thank you, this is very much appreciated. We'll try the PR as soon as it's merged.

However the workaround you mentioned does not work because then() is invoked immediately, unless we're missing something?

I'll ask one of our engineers to provide a code sample and explain the issue in more detail.

[johnatCB]

Here's the code sample

client.mutate({
            mutation: updatePage,
            variables: {page: newPage},
        }).then((result) => {
            client.mutate({
                mutation: toggleButton,
            }).then(() => {
                 another mutation is done here
            });
        });
...

The mutation doenst wait until it finish up to fire the result and the other mutation happens immediately after the mutation is triggered...

I've tried with #3169 and everything is working as expected. The mutation finishes and the other is triggered as it normally would.

[hwillson]

@johnatCB The problem with your code sample is that you're not returning a Promise from your then. So what you're really doing is creating a new unattached Promise sequence, instead of linking them together. If you return from your then, they will run in order.

Just to clarify though - the issue you've outlined in #1618 (comment) is not the same as the originally reported issue here. Your example has to do with Promise chaining issues, not that refetched queries aren't completed before the mutation promise resolves. PR #3169 is intended to fix this second case.

[johnatCB]

My bad thought it was related, thanks again @hwillson.

[hwillson]

#3169 addresses this, and has been merged. Please note that by default existing refetchQueries behaviour remains the same. You have to set the new awaitRefetchQueries option to true, to have refetched queries complete before the mutation is resolved. These changes should be published tomorrow. Thanks!

[DanielLaberge]

Thank you for the quick turn around, Hugh.

[Maarondesigns]

I agree that the refetchQueries issue needs to be resolved, however for me, I just avoid using it and chain promises like this:

this.props.myMutation( { variables: { myVariables } } ) 
.then( () => this.props.myQuery.refetch() ) 
.then( data => data.data.newQueryDataShouleBeUpdated )

Furthermore, awaitRefetchQueries option true did not change any behavior with .then() firing before the refetchQueries completed

[rwilliams3088]

+1 The mutate promise should complete before returning. This is important for error handling!!!! What if the mutation fails - such as due to a connection error? I need to respond to that. Whatever was initially intended, it's clearly an unintuitive and counterproductive design.