-
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
allow unknown query in refetchQueries, warn but continue #700
allow unknown query in refetchQueries, warn but continue #700
Conversation
@jasonphillips: 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/ |
I added one further test just now for the case of a correctly named (known) query that is asked to refetch but that has no active subscriptions. Based on the issue discussions, the only case for a useful warning is to indicate possible misnaming of the query; when the given query name is known but not actively subscribed, I wanted to ensure no warning ensues. |
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.
Cool, thanks for the PR! Could you change the tests a bit and then rebase it?
} else if (resultsReceived === 1) { | ||
assert.deepEqual(result.data, secondReqData); | ||
assert.include(warned[0], 'Warning: unknown query with name fakeQuery'); | ||
console.warn = oldWarn; |
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.
If you set it back here, console.warn
will be broken for all other tests if this one failed. I think it would be better to just override it before queryManager.mutate
and set it back immediately after the warning should have been logged.
// no warning should have been fired | ||
setTimeout(() => { | ||
assert.equal(timesWarned, 0); | ||
console.warn = oldWarn; |
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.
Same here with console.warn
staying broken if the test fails.
e5cf34e
to
4d87aa3
Compare
Thanks, I went ahead and moved the console.warn mocking to a cleaner |
Awesome, thanks a lot! |
Fix for bug identified in #690 and #594 (in the latter, some further API possibilities are discussed, but this addresses only the narrower case of #690).