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

On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira <
jdealmeidapereira(at)pivotal(dot)io> wrote:

> 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?
>
Yes - from me for 0002.

-- Thanks, Ashesh

> 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
>>>
>>
>>