-
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
await refetched queries before resolve mutation #3169
Conversation
@jzimmek: 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/ |
this pull request is adressing #1618 |
} | ||
|
||
this.query({ | ||
await this.query({ |
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.
Would it make sense to let these queries run in parallel, rather than await
ing each one after the other?
optimisticResponse, | ||
}); | ||
complete: async () => { | ||
try { |
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.
One way to avoid this try
-catch
wrapping would be to use an async
helper method:
complete: () => this._completeHelper().then(resolve, reject),
_completeHelper: async () => {
if (error) {...}
...
return storeResult;
}
You might want to rebase this PR and add a quick CHANGELOG, just to get those tests passing. Not sure why the bundle size would have changed at all… |
Any progress on this PR? Tried it myself with some changes (a complete helper async function, an await Promise.all in refetchqueries along with an await in each refetch both with string or document input) and seems to work nicely. Of course it executes the refetches in serial mode. |
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.
Thanks for your work on this @jzimmek!
We should be all set here, so it's LGTM time. Quick note - |
Fixes #1618.
Fixes #1821.
Fixes #3113.
Checklist: