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

On Mon, May 14, 2018 at 6:10 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

>
>
> 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 :-)
>
Yes - already working on it! :-)

-- Thanks, Ashesh

>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>