Re: [pgadmin4][patch] Initial patch to decouple from ACI Tree

Hello Ashesh,

1. In TreeNode, we're keepging the reference of DOMElement, do we really
>> need it?
>
> As of right now, our Tree abstraction acts as an adapter to the aciTree
>> library. The aciTree library needs the domElement for most of its functions
>> (setInode, unload, etc). Thus this is the easiest way to introduce our
>> abstraction and keep the functionality as before - at least until we decide
>> that whether we want to switch out the library or not.
>
> I understand that. But - I've not seen any reference of domElement the
> code yet, hence - pointed that out.

If you look at the function: reload, unload you will see that domNode is
used to communicate with the ACITree
​

> 2. Are you expecting the tree class to be a singleton class
>
> Since this tree is referenced throughout the codebase, considering it to
>> be a singleton seems like the most appropriate pattern for this usecase. It
>> is very much the same way how we create a single instance of the aciTree
>> library and use that throughout the codebase. Moreover, it opens up
>> opportunities to improve performance, for example caching lockups of nodes.
>> I’m not a fan of singletons myself, but I feel like we’re simply keeping
>> the architecture the same in the instance.
>
> Yeah - I don't see any usage of tree object from anywhere.
> And, we're already creating new object in browser.js (and, not utitlizing
> that instance anywhere.)

You are right, we do not need to export tree as a singleton for now. The
line that exports the variable tree can be remove when applying the patch
number 2.
​

I think we addressed all the concern raised about this patch. Does this
mean that the patch is going to get committed?

We are looking forward to continue the development on this track of work,
but as you know, we are blocked until it gets committed into master. We
hope that this gets unblocked soon because this is a very big track of work
and the longer it is blocked, the longer it will take to get finalized.

Thanks
Joao

On Fri, May 11, 2018 at 2:32 AM Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com>
wrote:

> On Thu, May 10, 2018 at 8:08 PM, Anthony Emengo <aemengo(at)pivotal(dot)io>
> wrote:
>
>> 1. In TreeNode, we're keepging the reference of DOMElement, do we really
>>> need it?
>>
>> As of right now, our Tree abstraction acts as an adapter to the aciTree
>> library. The aciTree library needs the domElement for most of its functions
>> (setInode, unload, etc). Thus this is the easiest way to introduce our
>> abstraction and keep the functionality as before - at least until we decide
>> that whether we want to switch out the library or not.
>>
> I understand that. But - I've not seen any reference of domElement the
> code yet, hence - pointed that out.
>
>> 2. Are you expecting the tree class to be a singleton class
>>
>> Since this tree is referenced throughout the codebase, considering it to
>> be a singleton seems like the most appropriate pattern for this usecase. It
>> is very much the same way how we create a single instance of the aciTree
>> library and use that throughout the codebase. Moreover, it opens up
>> opportunities to improve performance, for example caching lockups of nodes.
>> I’m not a fan of singletons myself, but I feel like we’re simply keeping
>> the architecture the same in the instance.
>>
> Yeah - I don't see any usage of tree object from anywhere.
> And, we're already creating new object in browser.js (and, not utitlizing
> that instance anywhere.)
>
> -- Thanks, Ashesh
>
>> ​
>>
>> Sincerely,
>>
>> Anthony and Victoria
>>
>
>