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

On Mon, May 14, 2018 at 1:38 PM, Ashesh Vashi <ashesh(dot)vashi(at)enterprisedb(dot)com
> wrote:

> On Mon, May 14, 2018 at 2:59 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>>
>>
>> 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?
>>
> Done.
>

Great, thanks!

On to patch 0003 then :-)

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

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