On Sat, May 12, 2018 at 12:10 AM, Ashesh Vashi < ashesh.va...@enterprisedb.com> wrote:
> On Sat, May 12, 2018, 02:58 Joao De Almeida Pereira < > jdealmeidapere...@pivotal.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
