Skip to content

Commit

Permalink
chore: change the klona strategy of setting of values to eval Tree (#…
Browse files Browse the repository at this point in the history
…38033)

## Description
Changed the klona strategy of evalActionBindings and parsed values
within evaluateTree to use klonaJSON instead of klona. Seeing a 10-20%
reduction in webworker scripting and a 4 second drop in LCP within a
windows machine.

Fixes #`Issue Number`  
_or_  
Fixes `Issue URL`
> [!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  -->
> [!IMPORTANT]
> 🟣 🟣 🟣 Your tests are running.
> Tests running at:
<https://github.com/appsmithorg/appsmith/actions/runs/12275133735>
> Commit: dbe08f9
> Workflow: `PR Automation test suite`
> Tags: `@tag.All`
> Spec: ``
> <hr>Wed, 11 Dec 2024 11:10:47 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

- **New Features**
- Enhanced error handling during data evaluation, providing better
context for debugging.
- Improved logic for evaluating dynamic properties within the data tree.

- **Bug Fixes**
- Addressed issues with data cloning methods to ensure more reliable
data handling.

- **Refactor**
- Updated method signatures for improved clarity and consistency across
the evaluation processes.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->
  • Loading branch information
vsvamsi1 authored Dec 11, 2024
1 parent 61c90ce commit 0a046ed
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 9 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -593,8 +593,8 @@ describe("DataTreeEvaluator", () => {
);
evaluator.evalAndValidateFirstTree();
// Hard check to not regress on the number of clone operations. Try to improve this number.
expect(klonaFullSpy).toBeCalledTimes(40);
expect(klonaJsonSpy).toBeCalledTimes(3);
expect(klonaFullSpy).toBeCalledTimes(15);
expect(klonaJsonSpy).toBeCalledTimes(28);
});

it("Evaluates a binding in first run", () => {
Expand Down Expand Up @@ -1115,7 +1115,7 @@ describe("DataTreeEvaluator", () => {
unEvalUpdates,
);
// Hard check to not regress on the number of clone operations. Try to improve this number.
expect(klonaFullSpy).toBeCalledTimes(6);
expect(klonaJsonSpy).toBeCalledTimes(2);
expect(klonaFullSpy).toBeCalledTimes(4);
expect(klonaJsonSpy).toBeCalledTimes(4);
});
});
10 changes: 5 additions & 5 deletions app/client/src/workers/common/DataTreeEvaluator/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1207,7 +1207,7 @@ export default class DataTreeEvaluator {
);

set(contextTree, fullPropertyPath, parsedValue);
set(safeTree, fullPropertyPath, klona(parsedValue));
set(safeTree, fullPropertyPath, klonaJSON(parsedValue));

staleMetaIds = staleMetaIds.concat(
getStaleMetaStateIds({
Expand Down Expand Up @@ -1253,7 +1253,7 @@ export default class DataTreeEvaluator {
if (!requiresEval) continue;

set(contextTree, fullPropertyPath, evalPropertyValue);
set(safeTree, fullPropertyPath, klona(evalPropertyValue));
set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue));
break;
}
case ENTITY_TYPE.JSACTION: {
Expand Down Expand Up @@ -1290,7 +1290,7 @@ export default class DataTreeEvaluator {
* Their evaluated values need to be reset only when the variable is modified by the user.
* When uneval value of a js variable hasn't changed, it means that the previously evaluated values are in both trees already */
if (!skipVariableValueAssignment) {
const valueForSafeTree = klona(evalValue);
const valueForSafeTree = klonaJSON(evalValue);

set(contextTree, fullPropertyPath, evalValue);
set(safeTree, fullPropertyPath, valueForSafeTree);
Expand All @@ -1305,7 +1305,7 @@ export default class DataTreeEvaluator {
}
default:
set(contextTree, fullPropertyPath, evalPropertyValue);
set(safeTree, fullPropertyPath, klona(evalPropertyValue));
set(safeTree, fullPropertyPath, klonaJSON(evalPropertyValue));
}
}
} catch (error) {
Expand Down Expand Up @@ -1793,7 +1793,7 @@ export default class DataTreeEvaluator {
bindings: string[],
executionParams?: Record<string, unknown> | string,
) {
const dataTree = klona(this.evalTree);
const dataTree = klonaJSON(this.evalTree);
// We might get execution params as an object or as a string.
// If the user has added a proper object (valid case) it will be an object
// If they have not added any execution params or not an object
Expand Down

0 comments on commit 0a046ed

Please sign in to comment.