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

Stale variables when combining `pollInterval`, `skip`, `forceFetch` #374

Closed
jbinto opened this Issue Dec 20, 2016 · 5 comments

Comments

Projects
None yet
2 participants
@jbinto
Copy link

jbinto commented Dec 20, 2016

This seems similar to #358 (but possibly unrelated).

Here is my situation:

  • When I click a button, I issue a mutation. I get an ID back (e.g. id=1).
  • A polling query starts (since the value of a skip became false). options is called with ownProps, and that has the ID from the mutation (e.g. id=1).
  • Polling begins, and network inspector shows the query is using the correct ID (e.g. id=1)
  • After the polling query sees changes in some state, we flip skip again and polling stops.
  • All is well at this point.

But, if I click the button again, this happens:

  • The mutation is issued. I get a new ID back (e.g. id=2)
  • A polling query starts (since the value of a skip became false). options is called with ownProps, and that has the ID from the mutation (e.g. id=2).
  • Polling begins, and network inspector shows the query is using the WRONG ID, from the previous variables. (e.g. id=1)
  • This breaks other things in my app because it was not expecting a result for id=1 but for id=2.

Steps to Reproduce

I've isolated this in a reproduction case:

git clone https://github.com/jbinto/apollo_polling_bug/
cd apollo_polling_bug
yarn
npm start

To keep things simple I don't do a mutation, just controlling props with setTimeout is sufficient to reproduce. I used a mock network interface that just echoes back an argument.

Relevant files:

https://github.com/jbinto/apollo_polling_bug/blob/master/src/index.js
https://github.com/jbinto/apollo_polling_bug/blob/master/src/Foo.js

Buggy Behavior

Somehow, apollo-client or react-apollo is holding on to old variables and issuing queries based on those, despite the fact the props of the graphql HOC have changed.

Expected Behavior

I should be able to combine skip, forceFetch, and pollInterval as such:

  • forceFetch and pollInterval are fixed as true, and 1000 respectively. skip begins as true.
  • Set skip to false, by re-rendering with new props (including an id prop, which is used in options.variables)
  • Let it poll for a bit
  • Set skip to true, polling stops
  • Let it idle for a bit
  • Set skip to false, and send a new id prop
  • Polling starts again, but...
  • It should use the new id prop I provided to options.variables, not the old one

Version

  • apollo-client@0.5.23
  • react-apollo@0.7.1
@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 21, 2016

Thank you @jbinto for the excellent reproduction.

Can confirm the problem and also that it's not AC (see this branch: https://github.com/tmeasday/apollo_polling_bug/tree/pure-apollo-client)

@tmeasday tmeasday added the bug label Dec 21, 2016

@tmeasday tmeasday self-assigned this Dec 21, 2016

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 21, 2016

Ok, so I think the issue is here: https://github.com/apollostack/react-apollo/blob/master/src/graphql.tsx#L343-L345

It assumes that we've either (a) just created the query with the current options and need to create a subscription or (b) we may be changing the options and the subscription should exist.

This is obviously third case that needs to be dealt with.

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 21, 2016

Started working on a solution, hit a couple of snags, will try and resolve the snags soon unless someone else solves them for me: #376

@tmeasday

This comment has been minimized.

Copy link
Contributor

tmeasday commented Dec 21, 2016

(Also this is quite annoying: #375)

@tmeasday tmeasday closed this Dec 22, 2016

@jbinto

This comment has been minimized.

Copy link
Author

jbinto commented Dec 22, 2016

Thanks for the quick turnaround!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment