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

On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi <
ashesh(dot)vashi(at)enterprisedb(dot)com> wrote:

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

Can you do that today?

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise PostgreSQL Company