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

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
>