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

Use flat structure from GraphQL query directly, avoid duplicating data #354

Closed
kinow opened this issue Jan 6, 2020 · 9 comments
Closed
Assignees
Milestone

Comments

@kinow
Copy link
Member

kinow commented Jan 6, 2020

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

PR #345 aims to improve the performance by keeping less items in the DOM, with a virtual scroller. The scroller is a Vue library. The library that does the magic of keeping less items in the DOM (using a view approximately the size of your current window viewport, with a buffer) is called Infinite Tree.

When you use the Infinite Tree, you provide a data structure with hierarchy. It then uses flattree library, maintained by the same developer. flattree, as per its name, simply gets the hierarchical data and returns a flat list.

The nodes in the list are in order, and contains certain attributes required to display them in the Infinite Tree.

Our current problem with this approach is that our GraphQL query may result in a large JS object in memory. And having this data duplicated by the Infinite Tree is not really the best scenario performance-wise.

So this issue is to investigate possible ways to avoid it.

Additional context

The hierarchical data structure is pretty close to what we had before:

{
  id: 1,
  name: 'foo',
  state: {}, // this is not GraphQL state, but a field used by InfiniteTree (!)
  children: [
    {
      id: ...
    }
  ]
}

Here's the constructor called when we provide the Infinite Tree object with a hierarchical data structure: https://github.com/cheton/infinite-tree/blob/7bf037ee79b8aa702f5ee605b8ec218bab2533b4/src/infinite-tree.js#L311

At the end of the method, it will call loadData, defined here: https://github.com/cheton/infinite-tree/blob/7bf037ee79b8aa702f5ee605b8ec218bab2533b4/src/infinite-tree.js#L1038

loadData is calling flatten from the flattree library.

Here's what the flat tree structure looks like:

{
  path: '1.1',
  parent: null,
  label: 'foo',
  state: {
    total: 0,
    depth: 1,
    open: false,
    prefixMask: ''
  }
}

The challenges here are:

  • Getting the GraphQL response to look as above
    • It might be tempting to just modify the existing GraphQL response, but that may actually break other components and create code smell/mess
  • Create an instance of the Infinite Tree object without the hierarchical data but with this new flat structure created by us
    • Need to make sure that events are triggered, and other states are properly initialized

gist with some example GraphQL data and the data used internally by the InfiniteTree (after flattree was used).

Pull requests welcome!

@kinow
Copy link
Member Author

kinow commented Jan 6, 2020

Probably a follow-up for #345 as this would take a bit to implement, and I suspect this is not on the roadmap for 0.2.

@kinow kinow added this to the 1.0 milestone Jan 6, 2020
@oliver-sanders
Copy link
Member

https://zen-of-python.info/flat-is-better-than-nested.html#5

@kinow
Copy link
Member Author

kinow commented Jan 30, 2020

Reading the infinite-tree source code today. When we create an InfiniteTree, we have an option to pass the data in the constructor. That's what we are doing right now.

The constructor then calls loadData. That's where the data with hierarchy gets flatten out. I thought we could maybe just skip it and assign the flat data instead, but looks like there's a bit more of work done after the flat data is set.

image

@kinow
Copy link
Member Author

kinow commented Jan 30, 2020

The only simple option that I found without having to hack the library, was the shouldLoadNodes + loadNodes.

The first function is used to tell the library whether it needs to load nodes or not.

The second function loads nodes dynamically.

Internally, I cannot see any call to the flattree library. It seems to expect users to pass an array with the flat data.

If we manage to get the data from GraphQL in a flat structure, already in order as it should be displayed, then we may just need to hook it up with the loadNodes function 👍

@kinow
Copy link
Member Author

kinow commented Jan 30, 2020

The InfiniteTree has also methods like appendChildNode and removeChildNode that can be used with the deltas, to add new nodes or remove nodes from the tree.

So the first GraphQL query could result in an new InfiniteTree with no data, but passing options like { loadNodes: someMethodToLoadNodesFromGraphQLData, ... }.

Then every time the subscribeToMore is invoked for the deltas subscription, it would check what needs to be added or removed, and update the Vuex store.

We would need a watcher in the Tree component then, to listen to the changes in the Vuex store (with deep: true I think), and then call

  • this.infiniteTree.removeChildNode(...) to remove nodes
  • this.infiniteTree.appendChildNode(...) to add nodes, or
  • this.infiniteTree.nodes.filter((node) => node.id === updatedNode.id)) or something similar to find an existing node and update its state (e.g. status, job information, etc.

When we are updating it, we can selectively skip state too, like not touching the state.open as a user may have changed it already - for #391

@kinow
Copy link
Member Author

kinow commented Feb 12, 2020

Thanks to @dwsutherland just found an easier way to find the cycle points.

workflows {
  cyclePoints: familyProxies(ids: ["root"]) {
    cyclePoint
  }
}

🎉 thanks @dwsutherland !

@kinow kinow self-assigned this Feb 12, 2020
@kinow kinow modified the milestones: 1.0, 0.3 Feb 12, 2020
@kinow
Copy link
Member Author

kinow commented Feb 12, 2020

Second improvement by @dwsutherland to our GraphQL query to retrieve the tree view data:

familyProxies (exids: ["root"], sort: {keys: ["cyclePoint"]}) {
  id
  cyclePoint
}

We don't need the root family in the UI, and we have an if in the code ATM to ignore that. With this change, we can remove that if 👌

@kinow
Copy link
Member Author

kinow commented Mar 2, 2020

I think this can be closed now?

The code on master is already using { node : object }. Only id's are copied as for the infinite tree we need { id: '', node: object } (but even then strings are interned normally in JS, so that shouldn't matter).

@hjoliver
Copy link
Member

I think this can be closed now?

@oliver-sanders - can you confirm and close?

@kinow kinow modified the milestones: 0.3, 0.2 Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants