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

Design to bring common variable change tracking functionality into QueryObservable #554

Closed
tmeasday opened this issue Aug 18, 2016 · 4 comments
Assignees

Comments

@tmeasday
Copy link
Contributor

tmeasday commented Aug 18, 2016

Following up on #513, here is a "lowest common denominator" design for bringing common logic into Apollo Client Core.

Goals:

  1. Allow view integrations to "declaratively" send the current values of variables to an observed query, and only have it requery if they've changed.
  2. Allow a mechanism to get the current state of the query (include { loading: true }) without having to track it themselves.
  3. Allow view integrations to send (potential) "default" values of variables to queries without having to understand the query (from things like parent props).
  4. As above for mutations

[It's debatable that all of these goals are valuable]

Non-goals:

  1. Allowing changing the query [1]
  2. Changing the API of any integrations
  3. Directly adding support for RxJS or any other alternate observable API (we have a orthogonal plan around extensions to address that).

[1] The React integration API doesn't let you change queries, and it doesn't look like the Angular integration encourages you to either. This could be controversial? (of course this design allows the integration to unsubscribe and re-query if it wants)

Design

It seems the above can be achieved in a straightforward fashion by extending the QueryObservable API.

To achieve 1:

If a QueryObservable tracks the previous values of variables, we could have an API like:

// force defaults to true, but false means don't do anything 
// if the variables haven't changed
observable.refetch(variables, { force: false });

To achieve 2:

Add:

observable.currentData()
 //  -> returns `{ loading: true }` if no data is available, 
 // or what was last passed to `next` otherwise

This is possibly redundant and integrations can track this information really easily using the next callback, although then perhaps refetch needs to tell the integration if it's doing anything

To achieve 3 + 4:

Add and option to relevant methods client.watchQuery(), observable.refetch(), client.mutate etc allowSuperfluousVariables that allows integrations to blindly pass things like { ...ownProps, ...userVariables } into a call without needing to figure out which of ownProps are relevant.

One downside of this approach is that legitimate problems with userVariables won't be reported. Another approach would be to support passing both variables and defaultVariables (unchecked) to all these calls.

@tmeasday
Copy link
Contributor Author

I'm not sure if this is the best format to take comments, but I've assigned people to the issue that I think will be interested. Let's try adding comments here. If you don't have anything to say and just want to sign off on it, I guess just 👍 the original comment.

@stubailo
Copy link
Contributor

I would definitely prefer setVariables over refetch, since refetch to me implies actually hitting the server and getting updated data. Related: #272

Also, I think we already remove unused variables: #518

@kamilkisiela
Copy link
Contributor

Directly adding support for RxJS or any other alternate observable API (we have a orthogonal plan around extensions to address that).

apollo-client-rxjs is ready.

We've already implemented this in angular2-apollo (see here).

It could be even simpler, but we need to support the old observable:

return rxify(this.client.watchQuery)(options);

tmeasday added a commit that referenced this issue Sep 12, 2016
tmeasday added a commit that referenced this issue Sep 22, 2016
tmeasday added a commit that referenced this issue Sep 22, 2016
@stubailo
Copy link
Contributor

@tmeasday is this closed now that #635 is merged?

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 17, 2023
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

6 participants