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

Incremental updates with GraphQL deltas #420

Closed
kinow opened this issue Mar 2, 2020 · 13 comments · Fixed by #458
Closed

Incremental updates with GraphQL deltas #420

kinow opened this issue Mar 2, 2020 · 13 comments · Fixed by #458
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Mar 2, 2020

Describe exactly what you would like to see in an upcoming release

Avoid re-creating Vuex data unnecessarily. Subscribe to GraphQL delta queries, and update the Vuex store.

Components should react accordingly to the data changed.

Additional context

The main challenge now, is that everything is a subscription (we did that to replace the old polling service).

Maybe instead of polling or subscription, we can simply send one query, then another subscription to update what has changed.

This is a blocker for the infinite tree, because of the state being in the data, not in the component any more - #391 . Which means users may alter the state of the nodes in the tree, but once the data is updated with new state, that state overrides whatever the user has done, which is not desirable.

Pull requests welcome!

@kinow kinow added this to the 0.3 milestone Mar 2, 2020
@kinow kinow self-assigned this Mar 2, 2020
@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Note: we need to use Apollo Client's .watchQuery(), that will create a kind of Observable. When we subscribe to that Observable (subscribe here has nothing to do with GraphQL), we can then retrieve the data.

The object created by .watchQuery() (an Apollo Client type ObservableQuery), besides being an Observable, has also the method .subscribeToMore().

The documentation points users to this method, and this combination of a query via .watchQuery, and the GraphQL subscription via the .subscribeToMore().

(Found most of the above by navigating the source in the IDE, while reading VueApollo and Angular Apollo documentation. Hard to find a pure JS documentation with good examples. The Apollo Client docs seem to be tailored for React.)

Note 2: first changing the /#/tree/five view, as that's simpler, and isolates the Tree component, without the Lumino tabs code. The GraphQL query used for now is:

subscription {
  deltas (id: "kinow|five") {
    pruned {
      tasks
      jobs
    }
    workflow {
      id
      status
      tasks {
        id
        name
        meanElapsedTime
      }
      jobs {
        id
        state
        batchSysName
        batchSysJobId
      }
    }
  }
}

Will update the query above later if necessary. Also important to note that I'm using the pull requests from Cylc Flow and Cylc UI Server cylc/cylc-flow#3500 & cylc/cylc-uiserver#118 respectively.

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Work being done in the branch https://github.com/kinow/cylc-ui/tree/deltas-1

Got the query running once, and updating/hydrating the components. Then the subscribeToMore was invoked (by ApolloClient) and started receiving data from the server. For tomorrow: update the store.

Won't create a WIP PR as it might change a lot in the next days, which would spam other's notifications inbox.

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Note to self: check in the UI Server & Flow PR's later if a warning seen in the browser is being caused by the client or by the server. I guess it's related to not all fields present in the schema being returned to the client.

image

Harmless as long as we find a way to disable the browser console logging for that error, as I got heaps in just a few minutes running the UI.

@dwsutherland
Copy link
Member

Note to self: check in the UI Server & Flow PR's later if a warning seen in the browser is being caused by the client or by the server. I guess it's related to not all fields present in the schema being returned to the client.

Yes, the response won't be a valid GraphQL result.. By stripping the result of null fields, we are violating the "client gets exactly what it asked for".

Harmless as long as we find a way to disable the browser console logging for that error, as I got heaps in just a few minutes running the UI.

Would be nice if we could turn the validation off (for production at least).. Is all that logging expensive?

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Is all that logging expensive

When the console is hidden, it is not too bad. Ubless you get thousands of entries in a very short time.

With the console open it is a bit more expensive. And I vould feel the UI a bit slower (or maybe just my imagination).

But the main issue was that I had to scroll to check if my new code had broken anything and caused some warning or error, but it was quite hard to filter through every message from the subscription.

@dwsutherland
Copy link
Member

Is all that logging expensive

When the console is hidden, it is not too bad. Ubless you get thousands of entries in a very short time.

With the console open it is a bit more expensive. And I vould feel the UI a bit slower (or maybe just my imagination).

But the main issue was that I had to scroll to check if my new code had broken anything and caused some warning or error, but it was quite hard to filter through every message from the subscription.

There will be heaps of them! .. Maybe there's an option to not validate the response/result.

@hjoliver
Copy link
Member

hjoliver commented Mar 2, 2020

Hmm, that's annoying! (the validation). If it can't be turned off, perhaps we can put up a PR (with a switch) to the upstream codebase...

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

There will be heaps of them! .. Maybe there's an option to not validate the response/result.

Yup, I saw lots of them yesterday, and I was running only five & famillies2 (though with script = sleep 2 😅), so I closed everything before going home to make sure I wouldn't be surprised by my VM being too slow.

Hmm, that's annoying! (the validation). If it can't be turned off, perhaps we can put up a PR (with a switch) to the upstream codebase...

We should be able to turn that off. I will try to take care to turn off only that specific error, but that depends more on what the client offers - https://www.apollographql.com/docs/react/data/error-handling/#error-policies (:crossed_fingers:)

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Hmmm, tried setting to ignore and none as per the error policies doc above, both when creating the client in src/utils/graphql (for both query and watchQuery), as well as when creating the subscription (the .query, .watchQuery, .subscribe functions can also take a errorPolicy or error argument). But nothing stopped the error.

Attached a debugger, looked at the transpiled JS code, and found the TypeScript code that emits the error: https://github.com/apollographql/apollo-client/blob/f26b98cc8ad859bef4ec74d77fd4245b022cc60d/src/cache/inmemory/writeToStore.ts#L243

image

🤔

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

Doesn't seem to have a way to disable this. For now I think I will just ignore it and try to finish the deltas update to the Vuex store. If that works, then I can prepare a draft PR so others can look what it may look like in JS, and what would need to change (@oliver-sanders will probably have some ideas to implement that). And in the meantime I can prepare a PR for the Apollo Client to either use a flag to disable that, or to raise an error (which I think can be captured using the error callbacks and/or the errorPolicy).

Oh, and found someone else with the same issue ☝️ also using deltas, but with Java.

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

p.s.: quick hack to be able to keep developing without so many entries in the console output:

console.warn = function () {}

@dwsutherland
Copy link
Member

Maybe this warn once utility can be used somehow?
https://github.com/apollographql/apollo-client/blob/57920121f5e561997f33a3d1d127c6bf99dac54b/packages/apollo-utilities/src/util/warnOnce.ts

@kinow
Copy link
Member Author

kinow commented Mar 3, 2020

Maybe this warn once utility can be used somehow?
https://github.com/apollographql/apollo-client/blob/57920121f5e561997f33a3d1d127c6bf99dac54b/packages/apollo-utilities/src/util/warnOnce.ts

I think so! Will try to use this one when preparing the upstream PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants