Hi Joao, Can you please rebase the second patch?
Thanks, Khushboo On Tue, Apr 3, 2018 at 12:15 AM, Joao De Almeida Pereira < jdealmeidapere...@pivotal.io> wrote: > Hi Hackers, > > Attached you can find the patch that will start to decouple pgAdmin from > ACITree library. > This patch is intended to be merged after 3.0, because we do not want to > cause any entropy or delay the release, but we want to start the discussion > and show some code. > > This job that we started is a massive tech debt chore that will take some > time to finalize and we would love the help of the community to do it. > > *Summary of the patch:* > 0001 patch: > - Creates a new tree that will allow us to create a separation between > the application and ACI Tree > - Creates a Fake Tree (Test double, for reference on the available test > doubles: https://martinfowler.com/bliki/TestDouble.html) that can be used > to inplace to replace the ACITree and also encapsulate the new tree > behavior on our tests > - Adds tests for all the tree functionalities > > 0002 patch: > - Extracts, refactors, adds tests and remove dependency from ACI Tree on: > - getTreeNodeHierarchy > - on backup.js: menu_enabled, menu_enabled_server, > start_backup_global_server, backup_objects > - on datagrid.js: show_data_grid, get_panel_title, show_query_tool > - Start using sprintf-js as Underscore.String is deprecating sprintf > function > > This patch represents only 10 calls to ACITree.itemData out of 176 that > are spread around our code > > > *In Depth look on the process behind the patch:* > > We started writing this patch with the idea that we need to decouple > pgAdmin4 from ACITree, because ACITree is no longer supported, the > documentation is non existent and ACITree is no longer being actively > developed. > > Our process: > 1. We "randomly" selected a function that is part of the ACITree. From > this point we decided to replace that function with our own version. The > function that we choose was "itemData". > The function gives us all the "data" that a specific node of the tree find. > Given in order to replace the tree we would need to have a function that > would give us the same information. We had 2 options: > a) Create a tree with a function called itemData > Pros: > - At first view this was the simpler solution > - Would keep the status quo > Cons: > - Not a OOP approach > - Not very flexible > b) Create a tree that would return a node given an ID and then the node > would be responsible for giving it's data. > Pros: > - OOP Approach > - More flexible and we do not need to bring the tree around, just a node > Cons: > - Break the current status quo > > Given these 2 options we decided to go for a more OOP approach creating a > Tree and a TreeNode classes, that in the future will be renamed to > ACITreeWrapper and TreeNode. > > 2. After we decided on the starting point we searched for occurrences of > the function "itemData" and we found out that there were 303 occurrences of > "itemData" in the code and roughly 176 calls to the function itself (some > of the hits were variable names). > > 3. We selected the first file on the search and found the function that > was responsible for calling the itemData function. > > 4. Extracted the function to a separate file > > 5. Wrap this function with tests > > 6. Refactor the function to ES6, give more declarative names to variables > and break the functions into smaller chunks > > 7. When all the tests were passing we replaced ACITree with our Tree > > 8. We ensured that all tests were passing > > 9. Remove function from the original file and use the new function > > 10. Ensure everything still works > > 11. Find the next function and execute from step 4 until all the functions > are replaced, refactored and tested. > > As you can see by the process this is a pretty huge undertake, because of > the number of calls to the function. This is just the first step on the > direction of completely isolating the ACITree so that we can solve the > problem with a large number of elements on the tree. > > *What is on our radar that we need to address:* > - Finish the complete decoupling of the ACITree > - Performance of the current tree implementation > - Tweak the naming of the Tree class to explicitly tell us this is to use > only with ACITree. > > > Thanks > Joao > >