-
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
Reset store returns a promise #1674
Conversation
@abergenw: 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/ |
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.
Hey @abergenw, thanks a lot! If you make the test a bit more complete I think it will be ready to merge.
test/QueryManager.ts
Outdated
@@ -2343,6 +2343,11 @@ describe('QueryManager', () => { | |||
}); | |||
|
|||
describe('store resets', () => { | |||
it('returns a promise', done => { | |||
const queryManager = createQueryManager({}); | |||
queryManager.resetStore().then(() => done()); |
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 should check that all queries have completed refetching before the then
is called.
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.
I think the basic setup of the test is great, I would just change how it actually determines that the refetch happened. Using readQuery
should be a lot more stable.
Also, make sure to associate your e-mail address with github so the commits will be assigned to you 😉
test/QueryManager.ts
Outdated
), | ||
}); | ||
|
||
let count = 0; |
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.
I'm a bit concerned that using count across these two queries makes this test really likely to fail on unrelated changes because it bakes in the order in which these events are fired, which might not always stay the same. How about just running two watchQueries, then refetching when they're both ready, and then in the then
callback of reset check that client.readQuery
for the two queries returns the second response for each? readQuery
is synchronous, so that should be fine.
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.
Done
Looks great, thanks a lot @abergenw ! |
Merged 🎉 Can you update the docs @abergenw ? |
TODO: