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

Add networkStatus to queries. #707

Closed
tmeasday opened this issue Sep 26, 2016 · 6 comments
Closed

Add networkStatus to queries. #707

tmeasday opened this issue Sep 26, 2016 · 6 comments
Assignees

Comments

@tmeasday
Copy link
Contributor

tmeasday commented Sep 26, 2016

Currently, in the store, queries have two fields that indicate their current "network status":

  1. loading: true/false: is there a network event attached to this query.
  2. returnPartialData: true/false: is there some data to return from this query (and does the query want it)?

Problems with this:

  1. There's a bit of a weird impedance mismatch between this and what the subscription publishes, as it does not publish loading: true states. Users should be able to read directly from the store and see results w/ the same shape as the subscription IMO.
  2. It's not clear what the purpose of returnPartialData is, given you can just check observable.options.returnPartialData at read time if you care. (If you are looking at the store and wondering what the network status of the query is, this doesn't tell you anything as the query is probably fetching anyway).
    i. There may be a case for a queryStoreValue.resultsIncomplete in the noFetch case however, to tell the user that even though the query isn't loading, the results aren't all there.

Proposed solution:

Add queryStoreValue.networkStatus and be explicit about what's happening for a query.

A key insight is there can be only one network event per query at any time, so there is a precedence to these, and they should clobber each other[1]

  1. loading - the query is loading for the first time.
  2. setVariables - the variables of the query have changed, the current results are for the old query.
  3. fetchMore - we are 'fetching more' for this query
  4. refetch - we are refetching
  5. poll - we are polling
  6. ready - all data is here. We good. [edit]

Note that loading will be true for states 1&2.
Also note that in cases 2-5, the query will be returning the "old" or "stale" value of the query.

We should publish networkStatus fairly transparently, clean up the behavior of the loading flag, and remove the returnPartialResults flag.

Potential objections:

  1. During the setVariables network event we should not return the old query results (from the subscription event or observable.currentResult())

    I'm on the fence about this one. It's the current behavior of react-apollo and it helps in some use cases, but it doesn't really feel "correct". cc Add observableQuery setVariables #635

  2. It becomes a little more tricky to know if the query should return partial results, but I think this is fine.

Consequences for react-apollo and other integrations

If RA passes { loading, networkStatus, currentResult } straight through (more or less), then the behavior will change as follows:

  1. loading will no longer be true during during refetches. However networkStatus: refetch will be true. Alternatively the user can use external state + the promise result to track the refetch progress.
  2. Depending on the resolution to objection 1. above, it may be the case that currentResult will be empty during a setVariables event. This is why I'm inclined to leave the behavior of Add observableQuery setVariables #635 and make it the old query's result.

@jbaxleyiii double checking with you that the above makes sense?

[1] By this I mean if a higher precedence network event fires, it should cancel the existing event, and if a lower precedence event fires it should just be a no-op.

@jbaxleyiii
Copy link
Contributor

@tmeasday this is super interesting! First thought is I like the direction and someone can always ignore it when creating custom props if they want.

Let me think about it and I'll get you a full response later today!

@tmeasday
Copy link
Contributor Author

See: #690

@tmeasday
Copy link
Contributor Author

tmeasday commented Oct 4, 2016

@davidyaha
Copy link
Contributor

@tmeasday This is not done yet right?

@tmeasday
Copy link
Contributor Author

I think @helfer just shipped it actually: https://github.com/apollostack/apollo-client/blob/master/CHANGELOG.md#v050-preview3

But it doesn't yet trigger subscriber changes on every network status change unless you request it, I think.

@helfer
Copy link
Contributor

helfer commented Oct 25, 2016

Yeah, it's what's in #827. Not yet updated in react-apollo.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants