Skip to content

Commit

Permalink
chore: remove unevalTree (appsmithorg#34605)
Browse files Browse the repository at this point in the history
## Description
evalTreeWithChanges is sending a redundant property unevalTree in the
payload, we should remove this property to reduce the serialisation cost
of including this property in the payload.

Fixes appsmithorg#34766

> [!WARNING]  
> _If no issue exists, please create an issue first, and check with the
maintainers if the issue is valid._

## Automation

/ok-to-test tags="@tag.All"

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!CAUTION]
> 🔴 🔴 🔴 Some tests have failed.
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/9807127090>
> Commit: 73e226d
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=9807127090&attempt=1&selectiontype=test&testsstatus=failed&specsstatus=fail"
target="_blank">Cypress dashboard</a>.
> Tags: @tag.All
> The following are new failures, please fix them before merging the PR:
<ol>
>
<li>cypress/e2e/Regression/ClientSide/PartialImportExport/PartialExport_spec.ts
> <li>cypress/e2e/Regression/ClientSide/Workspace/MemberRoles_Spec.ts
>
<li>cypress/e2e/Regression/ClientSide/Workspace/ShareAppTests_Spec.ts</ol>
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/identified-flaky-tests-65890b3c81d7400d08fa9ee3?branch=master"
target="_blank">List of identified flaky tests</a>.
> <hr>Fri, 05 Jul 2024 11:46:05 UTC
<!-- end of auto-generated comment: Cypress test results  -->






## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [ ] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

- **Refactor**
- Improved the handling of `unevalTree` by accessing it from
`unEvalAndConfigTree` instead of directly from the message data in
message handling logic.

- **Tests**
- Updated tests to reflect the removal of the `unevalTree` property and
related assertions.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
vsvamsi1 authored and Shivam-z committed Jul 10, 2024
1 parent 175b4a6 commit 7834fb7
Show file tree
Hide file tree
Showing 3 changed files with 2 additions and 30 deletions.
6 changes: 2 additions & 4 deletions app/client/src/sagas/EvalWorkerActionSagas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,15 +19,13 @@ import {
import { handleStoreOperations } from "./ActionExecution/StoreActionSaga";
import type { EvalTreeResponseData } from "workers/Evaluation/types";
import isEmpty from "lodash/isEmpty";
import type { UnEvalTree } from "entities/DataTree/dataTreeTypes";
import { sortJSExecutionDataByCollectionId } from "workers/Evaluation/JSObject/utils";
import type { LintTreeSagaRequestData } from "plugins/Linting/types";
import { evalErrorHandler } from "./EvalErrorHandler";
import { getUnevaluatedDataTree } from "selectors/dataTreeSelectors";

export interface UpdateDataTreeMessageData {
workerResponse: EvalTreeResponseData;
unevalTree: UnEvalTree;
}

export function* handleEvalWorkerRequestSaga(listenerChannel: Channel<any>) {
Expand Down Expand Up @@ -141,12 +139,12 @@ export function* handleEvalWorkerMessage(message: TMessage<any>) {
break;
}
case MAIN_THREAD_ACTION.UPDATE_DATATREE: {
const { unevalTree, workerResponse } = data as UpdateDataTreeMessageData;
const { workerResponse } = data as UpdateDataTreeMessageData;
const unEvalAndConfigTree: ReturnType<typeof getUnevaluatedDataTree> =
yield select(getUnevaluatedDataTree);
yield call(updateDataTreeHandler, {
evalTreeResponse: workerResponse as EvalTreeResponseData,
unevalTree,
unevalTree: unEvalAndConfigTree.unEvalTree || {},
requiresLogging: false,
configTree: unEvalAndConfigTree.configTree,
});
Expand Down
23 changes: 0 additions & 23 deletions app/client/src/workers/Evaluation/evalTreeWithChanges.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ describe("evaluateAndGenerateResponse", () => {

expect(parsedUpdates).toEqual([]);
expect(webworkerResponse).toEqual({
unevalTree: {},
workerResponse: {
dependencies: {},
errors: [],
Expand Down Expand Up @@ -224,28 +223,6 @@ describe("evaluateAndGenerateResponse", () => {

expect(parsedUpdates).toEqual([]);
});
test("should send the new unevalTree in the web worker response", () => {
const updatedLabelUnevalTree = produce(unEvalTree, (draft: any) => {
if (draft.Text1?.text) {
draft.Text1.text = UPDATED_LABEL;
}
});
expect(evaluator.getOldUnevalTree()).toEqual(unEvalTree);
const updateTreeResponse = evaluator.setupUpdateTree(
updatedLabelUnevalTree,
configTree,
);
// the new unevalTree gets set in setupUpdateTree
expect(evaluator.getOldUnevalTree()).toEqual(updatedLabelUnevalTree);

const { unevalTree } = evalTreeWithChanges.evaluateAndGenerateResponse(
evaluator,
updateTreeResponse,
[],
[],
);
expect(unevalTree).toEqual(updatedLabelUnevalTree);
});

describe("updates", () => {
test("should generate updates based on the unEvalUpdates", () => {
Expand Down
3 changes: 0 additions & 3 deletions app/client/src/workers/Evaluation/evalTreeWithChanges.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,6 @@ export const evaluateAndGenerateResponse = (
defaultResponse.evalMetaUpdates = [...(metaUpdates || [])];
return {
workerResponse: defaultResponse,
unevalTree: {},
};
}

Expand Down Expand Up @@ -134,7 +133,6 @@ export const evaluateAndGenerateResponse = (
);

defaultResponse.staleMetaIds = updateResponse.staleMetaIds;
const unevalTree = dataTreeEvaluator.getOldUnevalTree();

// when additional paths are required to be added as updates, we extract the updates from the data tree using these paths.
const additionalUpdates = getNewDataTreeUpdates(
Expand All @@ -159,7 +157,6 @@ export const evaluateAndGenerateResponse = (

return {
workerResponse: defaultResponse,
unevalTree,
};
};

Expand Down

0 comments on commit 7834fb7

Please sign in to comment.