CirclonGroup / angular-tree-component

A simple yet powerful tree component for Angular (>=2)
https://angular2-tree.readme.io/docs
MIT License
1.09k stars 488 forks source link

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

Open jdewell opened 6 years ago

jdewell commented 6 years ago

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:

{{ 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 commented 6 years ago

Also I have enabled prod mode.

adamkleingit commented 6 years ago

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

jdewell commented 6 years ago

The treeModel.

On Sep 5, 2017, at 12:52 AM, adam klein notifications@github.com<mailto:notifications@github.com> 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.

adamkleingit commented 6 years ago

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

jdewell commented 6 years ago

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

On Sep 5, 2017, at 8:07 AM, adam klein notifications@github.com<mailto:notifications@github.com> 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.

adamkleingit commented 6 years ago

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 commented 6 years ago

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
}); 
jdewell commented 6 years ago

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 notifications@github.com<mailto:notifications@github.com> 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.

johnking commented 6 years ago

Hi @adamkleingit

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

Looking forward to the fix.

thanks

supriyaraj03 commented 6 years ago

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

JuliaRakitina commented 6 years ago

+1

NPGiorgi commented 6 years ago

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!

jdewell commented 6 years ago

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

Thanks, -John

From: adam klein [mailto:notifications@github.com] Sent: Tuesday, September 05, 2017 8:35 AM To: 500tech/angular-tree-component angular-tree-component@noreply.github.com Cc: John Dewell John.Dewell@hci.utah.edu; Author author@noreply.github.com 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.

nshelms commented 6 years ago

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.

groothuyse commented 6 years ago

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);
      });
    }
  }
adamkleingit commented 5 years ago

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

Vanguard90 commented 5 years ago

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.