From 6c81f07e79720ce60327d6a71ce446d6dbc782ad Mon Sep 17 00:00:00 2001 From: arunvjn <32433245+arunvjn@users.noreply.github.com> Date: Tue, 28 Nov 2023 14:40:02 +0530 Subject: [PATCH] fix: deleted JSObject names are not released for later use (#29148) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Description > Repeated refactor operations seems to leave stale JS Objects in the dataTree causing the dataTree to occupy namespace that no longer exists. This seems to have stemmed from the `updateEvalTreeWithJSCollectionState` method in app/client/src/workers/common/DataTreeEvaluator/index.ts, where we update the existing evalTree with JSObjects and their variables from the previous unEvalTree. > This PR modifies `updateEvalTreeWithJSCollectionState` to not apply variable state to evalTree if the entity is not present. #### PR fixes following issue(s) Fixes #29162 > if no issue exists, please create an issue and ask the maintainers about this first > > #### Media > A video or a GIF is preferred. when using Loom, don’t embed because it looks like it’s a GIF. instead, just link to the video > > #### Type of change - Bug fix (non-breaking change which fixes an issue) > > ## Testing > #### How Has This Been Tested? - [x] Manual - [x] Cypress > > #### Test Plan - [x] Repeated renaming operations should work. - [x] Deleting a JS Object should make the name available for use. - [x] Execution data should be preserved on rename operations > > #### Issues raised during DP testing > Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR) > > > ## Checklist: #### Dev activity - [x] My code follows the style guidelines of this project - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [ ] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [ ] PR is being merged under a feature flag #### QA activity: - [ ] [Speedbreak features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-) have been covered - [ ] Test plan covers all impacted features and [areas of interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-) - [ ] Test plan has been peer reviewed by project stakeholders and other QA members - [ ] Manually tested functionality on DP - [ ] We had an implementation alignment call with stakeholders post QA Round 2 - [ ] Cypress test cases have been added and approved by SDET/manual QA - [ ] Added `Test Plan Approved` label after Cypress tests were reviewed - [ ] Added `Test Plan Approved` label after JUnit tests were reviewed --- .../ClientSide/BugTests/Bug29131_spec.ts | 57 +++++++++++++++++++ app/client/src/ce/sagas/JSActionSagas.ts | 20 +++---- app/client/src/sagas/ActionSagas.ts | 20 +++---- .../src/workers/Evaluation/JSObject/index.ts | 16 +----- .../workers/common/DataTreeEvaluator/index.ts | 2 +- 5 files changed, 81 insertions(+), 34 deletions(-) create mode 100644 app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29131_spec.ts diff --git a/app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29131_spec.ts b/app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29131_spec.ts new file mode 100644 index 0000000000..e6272c5115 --- /dev/null +++ b/app/client/cypress/e2e/Regression/ClientSide/BugTests/Bug29131_spec.ts @@ -0,0 +1,57 @@ +import { + agHelper, + locators, + entityExplorer, + jsEditor, +} from "../../../../support/Objects/ObjectsCore"; +import { EntityItems } from "../../../../support/Pages/AssertHelper"; + +const jsObjectBody = `export default { + myVar1: [], + myVar2: {}, + myFun1(){ + + }, + myFun2: async () => { + //use async-await or promises + } +}`; + +describe("Verifies JS object rename bug", () => { + it("Verify that a JS Object name is up for taking after it is deleted", () => { + jsEditor.CreateJSObject(jsObjectBody, { + paste: true, + completeReplace: true, + toRun: false, + shouldCreateNewJSObj: true, + prettify: false, + }); + + jsEditor.CreateJSObject(jsObjectBody, { + paste: true, + completeReplace: true, + toRun: false, + shouldCreateNewJSObj: true, + prettify: false, + }); + + jsEditor.RenameJSObjFromPane("JSObj2"); + + agHelper.ActionContextMenuWithInPane({ + action: "Delete", + entityType: EntityItems.JSObject, + }); + + jsEditor.CreateJSObject(jsObjectBody, { + paste: true, + completeReplace: true, + toRun: false, + shouldCreateNewJSObj: true, + prettify: false, + }); + + jsEditor.RenameJSObjFromPane("JSObj2"); + + entityExplorer.AssertEntityPresenceInExplorer("JSObj2"); + }); +}); diff --git a/app/client/src/ce/sagas/JSActionSagas.ts b/app/client/src/ce/sagas/JSActionSagas.ts index b7a9b6ddee..a416494a80 100644 --- a/app/client/src/ce/sagas/JSActionSagas.ts +++ b/app/client/src/ce/sagas/JSActionSagas.ts @@ -377,19 +377,19 @@ export function* refactorJSObjectName( getJSCollection(state, id), ); const functions = jsObject.actions; - yield put( - updateActionData( - functions.map((f) => ({ - entityName: newName, - data: undefined, - dataPath: `${f.name}.data`, - dataPathRef: `${oldName}.${f.name}.data`, - })), - ), - ); if (currentPageId === pageId) { // @ts-expect-error: refactorResponse.data is of type unknown yield updateCanvasWithDSL(refactorResponse.data, pageId, layoutId); + yield put( + updateActionData( + functions.map((f) => ({ + entityName: newName, + data: undefined, + dataPath: `${f.name}.data`, + dataPathRef: `${oldName}.${f.name}.data`, + })), + ), + ); } else { yield put(fetchJSCollectionsForPage(pageId)); } diff --git a/app/client/src/sagas/ActionSagas.ts b/app/client/src/sagas/ActionSagas.ts index dca0ed5015..7d7703942f 100644 --- a/app/client/src/sagas/ActionSagas.ts +++ b/app/client/src/sagas/ActionSagas.ts @@ -748,19 +748,19 @@ export function* refactorActionName( actionId: id, }, }); - yield put( - updateActionData([ - { - entityName: newName, - dataPath: "data", - data: undefined, - dataPathRef: `${oldName}.data`, - }, - ]), - ); if (currentPageId === pageId) { // @ts-expect-error: refactorResponse is of type unknown yield updateCanvasWithDSL(refactorResponse.data, pageId, layoutId); + yield put( + updateActionData([ + { + entityName: newName, + dataPath: "data", + data: undefined, + dataPathRef: `${oldName}.data`, + }, + ]), + ); } else { yield put(fetchActionsForPage(pageId)); } diff --git a/app/client/src/workers/Evaluation/JSObject/index.ts b/app/client/src/workers/Evaluation/JSObject/index.ts index 56ac11224f..f702ee125b 100644 --- a/app/client/src/workers/Evaluation/JSObject/index.ts +++ b/app/client/src/workers/Evaluation/JSObject/index.ts @@ -1,4 +1,4 @@ -import { get, isEmpty, merge, set } from "lodash"; +import { get, isEmpty, set } from "lodash"; import type { JSActionEntity } from "@appsmith/entities/DataTree/types"; import type { ConfigTree, DataTree } from "entities/DataTree/dataTreeTypes"; import { EvalErrorTypes, getEvalValuePath } from "utils/DynamicBindingUtils"; @@ -297,22 +297,12 @@ export function getJSEntities(dataTree: DataTree) { return jsCollections; } -export function updateEvalTreeWithJSCollectionState( - evalTree: DataTree, - oldUnEvalTree: DataTree, -) { +export function updateEvalTreeWithJSCollectionState(evalTree: DataTree) { // loop through jsCollectionState and set all values to evalTree const jsCollections = JSObjectCollection.getVariableState(); const jsCollectionEntries = Object.entries(jsCollections); for (const [jsObjectName, variableState] of jsCollectionEntries) { - if (!evalTree[jsObjectName]) { - evalTree[jsObjectName] = merge( - {}, - oldUnEvalTree[jsObjectName], - variableState, - ); - continue; - } + if (!evalTree[jsObjectName]) continue; evalTree[jsObjectName] = Object.assign( evalTree[jsObjectName], variableState, diff --git a/app/client/src/workers/common/DataTreeEvaluator/index.ts b/app/client/src/workers/common/DataTreeEvaluator/index.ts index 95087fc9bb..97adcb37af 100644 --- a/app/client/src/workers/common/DataTreeEvaluator/index.ts +++ b/app/client/src/workers/common/DataTreeEvaluator/index.ts @@ -714,7 +714,7 @@ export default class DataTreeEvaluator { updateDependencyMapTime = "0", } = extraParams; - updateEvalTreeWithJSCollectionState(this.evalTree, this.oldUnEvalTree); + updateEvalTreeWithJSCollectionState(this.evalTree); const calculateSortOrderStartTime = performance.now(); const subTreeSortOrder: string[] = this.calculateSubTreeSortOrder(