angular-tree-component icon indicating copy to clipboard operation
angular-tree-component copied to clipboard

treeModel.expandAll() never returns, bogs down chrome completely with a large tree.

Open jdewell opened this issue 8 years ago • 17 comments

Hi, I have been enjoying using angular-tree-component for my tree inplementation. I havent seen any problems until I tried to expandAll() on my tree. The call to treeModel.expandAll() never returns and completely bogs down chrome. My tree has about 30000 nodes in a 3 deep configuration - 500->3000->24000. I have turned on virtual scroll. The tree component has no performance issues initially displaying the collapsed tree. It takes just a second or so. Also scrolling is performant. Unfortunately, the expandAll() crashes.

Here are my options:

/*
angular2-tree options
 */
private options: ITreeOptions = {
    displayField: "label",
    childrenField: "items",
    useVirtualScroll: true,
    nodeHeight: 22,
    allowDrop: (element, { parent, index }) => {
        this.dragEndItems = _.cloneDeep(this.items);
        if (parent.data.labName) {
            return false;
        } else {
            return true;
        }
    },

    allowDrag: (node) => node.isLeaf,
};

HTML:

<tree-root #tree (moveNode)="onMoveNode($event)" (dragEnd)="onDragEnd1($event)" (activate)="treeOnSelect($event)" [nodes]="tinys" [options]="options"> <ng-template #treeNodeTemplate let-node let-index="index"> {{ node.data.label }}

Attached is my package.json package.json.txt

Attached is my example json. slowexpand.json.txt

I am using chrome Version 60.0.3112.113

Thank you. -John

jdewell avatar Sep 02 '17 21:09 jdewell

Also I have enabled prod mode.

jdewell avatar Sep 02 '17 21:09 jdewell

Hi, did u call expandAll on the treeModel or the treeNode?

adamkleingit avatar Sep 05 '17 06:09 adamkleingit

The treeModel.

On Sep 5, 2017, at 12:52 AM, adam klein <[email protected]mailto:[email protected]> wrote:

Hi, did u call expandAll on the treeModel or the treeNode?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/500tech/angular-tree-component/issues/415#issuecomment-327087253, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AI14N9fdxvBIOVtRFEMKfNr0D-8IjiCzks5sfO-egaJpZM4PLBUv.

jdewell avatar Sep 05 '17 14:09 jdewell

Yes I see the problem. I'll try to debug and appreciate if you also look into it

adamkleingit avatar Sep 05 '17 14:09 adamkleingit

Ok. Thanks for looking into this. I'll see what I can do.

On Sep 5, 2017, at 8:07 AM, adam klein <[email protected]mailto:[email protected]> wrote:

Yes I see the problem. I'll try to debug and appreciate if you also look into it

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/500tech/angular-tree-component/issues/415#issuecomment-327185839, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AI14N5kK3H4wDp6RGHZgAXGF3aYwKnfKks5sfVVFgaJpZM4PLBUv.

jdewell avatar Sep 05 '17 14:09 jdewell

I found the problem - expand returns a Promise.resolve, which turns out to be asynchronous even if it's an immediate resolve. This causes MobX reactions to run on each node expand and recalculate the entire tree's node positions for virtual scroll. I need to either debounce it or change the API so that it returns a synchronous Promise (or not a promise at all)

adamkleingit avatar Sep 05 '17 14:09 adamkleingit

As a bypass I suggest you do something like this:

import { transaction } from 'mobx';

transaction(() => {
  // loop over the nodes recursively and call expand() on each one
}); 

adamkleingit avatar Sep 05 '17 14:09 adamkleingit

Thanks much! I'm under the weather today but I'll try that when I get to the office.

Sent from my iPhone

On Sep 5, 2017, at 8:38 AM, adam klein <[email protected]mailto:[email protected]> wrote:

As a bypass I suggest you do something like this:

import { transaction } from 'mobx';

transaction(() => { // loop over the nodes recursively and call expand() on each one });

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/500tech/angular-tree-component/issues/415#issuecomment-327196089, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AI14N4SRQGwvxTR7roC0s5212gNQWM0Dks5sfVzKgaJpZM4PLBUv.

jdewell avatar Sep 05 '17 15:09 jdewell

Hi @adamkleingit

the issue #414 I submitted might be due to the same root cause.

Looking forward to the fix.

thanks

johnking avatar Sep 05 '17 19:09 johnking

Hi @adamkleingit

I am also using this tree view for my project and must say it is very easy to integrate and user friendly.

But we have the same issue: I have problems with the expandAll() on my tree. I have just a mock data to build this tree and have just 3 nodes with 10 child nodes underneath. The minute I click on expandAll button, it takes a very long time to expand. The performance is really slow.

I expect that we around 20,000 nodes and also 50 nodes under each node as a child node.

Would be really nice if the expandAll works good ! There is a filter option for this tree view and the performance of that is just so brilliant. Could we have the similar performance for expand all as well ?

I am really looking forward to the fix.

Thanks & Regards

supriyaraj03 avatar Sep 07 '17 12:09 supriyaraj03

+1

JuliaRakitina avatar Sep 08 '17 17:09 JuliaRakitina

Hi all,

We are having the same issue here, your plugin works great and I think it's the best tree view plugin for angular. If you could solve this issue it would really help us, as the only thing we are missing is the expand all feature to be performant.

I've tried removing some events that I'm not using and removing some of the promises to try to reduce the time it takes to render, but the time reduction wasn't significant so I'm not probably missing something else.

I'm really looking forward for this fix.

Thanks!

NPGiorgi avatar Oct 27 '17 13:10 NPGiorgi

Hi Adam, In recent versions have you addressed this issue?

Thanks, -John

From: adam klein [mailto:[email protected]] Sent: Tuesday, September 05, 2017 8:35 AM To: 500tech/angular-tree-component [email protected] Cc: John Dewell [email protected]; Author [email protected] Subject: Re: [500tech/angular-tree-component] treeModel.expandAll() never returns, bogs down chrome completely with a large tree. (#415)

I found the problem - expand returns a Promise.resolve, which turns out to be asynchronous even if it's an immediate resolve. This causes MobX reactions to run on each node expand and recalculate the entire tree's node positions for virtual scroll. I need to either debounce it or change the API so that it returns a synchronous Promise (or not a promise at all)

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/500tech/angular-tree-component/issues/415#issuecomment-327195114, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AI14N6cRKchrJruQPd2omL_KOJs39eqHks5sfVwJgaJpZM4PLBUv.

jdewell avatar Jan 09 '18 22:01 jdewell

In another issue it was suggested that the fireEvent was causing the performance issue in expandAll(). I tried commenting out the call in the following code but it didn't not seem any better. I'm working with about 1000 nodes of data with about 5 levels.

TreeModel.prototype.setExpandedNode = function (node, value) {
        this.expandedNodeIds = Object.assign({}, this.expandedNodeIds, (_a = {}, _a[node.id] = value, _a));
        //this.fireEvent({ eventName: TREE_EVENTS.toggleExpanded, node: node, isExpanded: value });
        var _a;
    };

@adamkleingit I'm not familiar with mobx, how would i further go about implementing your suggestion with transaction? Would love to help resolve this if you could point me in the right direction.

nshelms avatar Apr 25 '18 00:04 nshelms

2022 edit: fastest option is for sure: https://github.com/CirclonGroup/angular-tree-component/issues/415#issuecomment-445223646

I am dealing with the same issue. For people who may not have understood @adamkleingit 's suggestion. This snippet works well for me and performance is better than the default .expandAll() method:

add this import: import {transaction} from 'mobx';

  expandAll() {
    /* For enhanced performance until:
       https://github.com/500tech/angular-tree-component/issues/415
       is fixed, do a manual expansion instead of this.angularTree.treeModel.expandAll().
     */
    transaction(() => {
      this.angularTree.treeModel.getVisibleRoots().forEach(this.expandNode.bind(this));
    });
  }

  private expandNode(node: TreeNode) {
    node.expand();

    if (node.children) {
      node.children.forEach((child: TreeNode) => {
        this.expandNode(child);
      });
    }
  }

groothuyse avatar Jul 29 '18 19:07 groothuyse

@groothuyse - care to make a Pull Request and make this library better for everyone? Thanks!

adamkleingit avatar Aug 12 '18 08:08 adamkleingit

For people that are dealing with this problem, there is an another solution here, by amunra2112: https://github.com/500tech/angular-tree-component/issues/604

I tried all of the above solutions but the one that solved the problem for me is at that link.

Vanguard90 avatar Dec 07 '18 12:12 Vanguard90