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

Don't compare against cache if forceFetch is true #1264

Merged
merged 5 commits into from
Feb 16, 2017

Conversation

stevewillard
Copy link
Contributor

Fixes #1214

@apollo-cla
Copy link

@stevewillard: 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/

@@ -502,7 +502,7 @@ export class QueryManager {

// If this is not a force fetch, we want to diff the query against the
// store before we fetch it from the network interface.
if (!forceFetch) {
if (fetchType !== FetchType.refetch || !forceFetch) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it’s correct here to use && and not ||. See if that fixes the tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks -- seems to be passing now. I merged/squashed the commits.

@stevewillard
Copy link
Contributor Author

Ok - I'll try it out later tonight or tomorrow. Also, any idea why I can't sign the CLA? I keep getting a 500 error when I fill out all the fields.

@stubailo
Copy link
Contributor

stubailo commented Feb 7, 2017

@stevewillard I just changed some stuff about CLA let me check

@stubailo stubailo closed this Feb 7, 2017
@stubailo stubailo reopened this Feb 7, 2017
@calebmer
Copy link
Contributor

calebmer commented Feb 7, 2017

Looks like that fixed it. @stevewillard would you be willing to write a test to prevent future regressions? Otherwise it looks good to me 👍

@stevewillard
Copy link
Contributor Author

Hey, sorry about lagging on this PR. I am really swamped with work. I don't think I'll have time for a few weeks to finish this up. Happy to close it out if someone wants to take another stab at it.

@calebmer
Copy link
Contributor

I’m comfortable merging this without a test if it is going to be too much effort. @helfer what do you think?

@helfer
Copy link
Contributor

helfer commented Feb 16, 2017

Yeah, that works for me, especially since we might replace forceFetch and noFetch with an explicit fetch policy (more on that soon). I'll merge it now! 🙂 Thanks @stevewillard!

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.

5 participants