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

Changing variables in watchQuery after polling started #272

Closed
timsuchanek opened this issue Jun 7, 2016 · 11 comments
Closed

Changing variables in watchQuery after polling started #272

timsuchanek opened this issue Jun 7, 2016 · 11 comments

Comments

@timsuchanek
Copy link

timsuchanek commented Jun 7, 2016

Hi,
so I'm creating an observable with the client.watchQuery method and watch on it.
It works fine.
But let's say variables in my query changed.

How to properly handle that?
Do I really have to kill the old observable and create a new one which includes the new query?

In other words: How to change variables of a polling ObservableQuery in runtime?

Idea: Pass in an optional callback that returns the current variables.

I ask, because doing that kill/respawn approach would require a lot of refactoring in my code.

Thanks!

@Poincare
Copy link
Contributor

Poincare commented Jun 7, 2016

Hi Tim. If you check out the relevant portion in src/QueryManager.ts:

 public watchQuery(options: WatchQueryOptions): ObservableQuery {
    // Call just to get errors synchronously
    getQueryDefinition(options.query);

    return new ObservableQuery((observer) => {
      const queryId = this.startQuery(options, (queryStoreValue: QueryStoreValue) => {
        if (!queryStoreValue.loading || queryStoreValue.returnPartialData) {

Since options is an object and you'd be passing a reference to the object itself, this could mean that just updating the keys/values in the object might be sufficient to change the variables the next time the query fires. This should be pretty easy to test - could you try it out?

Whether or not it works, it would be great if you could distill it into a unit test that we can use to guide this issue. Check out test/QueryManager.ts - that seems like a natural place for this unit test. Basically, you can use the mockNetworkInterface, set up a watchQuery and then modify one of the variables and assert whether or not the query was changed correctly.

@stubailo
Copy link
Contributor

stubailo commented Jun 7, 2016

@Poincare I think mutating the variables is not the right approach long term, even though it might work now. I think we should encourage people to make those immutable, and perhaps provide a method to update the query options.

@Poincare
Copy link
Contributor

Poincare commented Jun 8, 2016

Makes sense. I guess I will leave this issue open and we'll fix it soon.

@timsuchanek
Copy link
Author

Hey guys, thanks for the fast answers.
@Poincare your solution works like a charm.
So do you want some unit tests or better later when there is a better api for this use case?

@stubailo
Copy link
Contributor

stubailo commented Jun 8, 2016

You can help us design the API! What should it look like?

@timsuchanek
Copy link
Author

timsuchanek commented Jun 11, 2016

Ha I'm pleasured to tell you my opinion for the API.

Ok, this is my use case:
I get params from react-router into my React component and pass them into an action, which is a redux-thunk, that then creates the observable and its subscription, parametrized by the params passed into the action by the component.

(Btw I'm aware of react-apollo, but I had some problems with error handling with it, so I decided for a pure redux/apollo-client implementation, as I just could move forward faster with this approach.)

So the first naive Idea I had, was to pass the apollo client a callback, that looks into the redux store and gets what it needs. Problem is: The url params of react-router are not available in the redux store.
They only get passed in into the component directly.
So we need a way that includes the component.
So the way I suggest is, that we add an explicit call on the observable, that updates the observable and if the user wants, directly performs a query afterwards.

Something like observable.updateVariables({vars}) would be really handy in this scenario.
Would do you think about it?

@stubailo
Copy link
Contributor

Just trying to understand better - why not stop the previous observable and start a new one with new variables?

@Akryum
Copy link

Akryum commented Jul 21, 2016

Isn't this less efficient in terms of performance/memory?

@stubailo
Copy link
Contributor

Not really - the overhead of creating an observable object is basically 0, everything else is handled by the cache almost exactly the same way.

@stubailo
Copy link
Contributor

Or yeah, I guess there are also some extra things stored in Redux when you start/stop a query. So it could be less efficient, but we haven't run into that as a source of problems in our production apps. It's mostly React renders that are very costly.

@stubailo
Copy link
Contributor

Done! #635

@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

4 participants