fix: All property control renders when JS toggle button is clicked #18905

aswathkk · 2022-12-13T02:24:47Z

Description

TL;DR: Fix the performance issue that happens when we click the JS toggle button in property pane.

Each property controls were getting rendered whenever we click the JS toggle button. This is since one of the selector used in PropertyControl was returning dynamicPropertyPathList which will always change when we click JS toggle button anywhere. Now, instead of returning dynamicPropertyPathList from that selector, we are returning a derived boolean value that is specific for the PropertyControl which calls this selector.
Similar behaviour was noticed with dynamicTriggerPathList as well. That is also resolved in a similar manner.

While working on this, I noticed the ActionCreator was also rendering since we aren't using proper selectors. That is also fixed.

Introduced proxy-momoize library which is an alternative to reselect's createSelector. This makes use of ES6 proxies to do an efficient memoization.

Fixes #11040

Type of change

Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

Manual

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

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have commented my code, particularly in hard-to-understand areas
I have made corresponding changes to the documentation
My changes generate no new warnings
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes
PR is being merged under a feature flag

QA activity:

Test plan has been approved by relevant developers
Test plan has been peer reviewed by QA
Cypress test cases have been added and approved by either SDET or manual QA
Organized project review call with relevant stakeholders after Round 1/2 of QA
Added Test Plan Approved label after reveiwing all Cypress test

vercel · 2022-12-13T02:24:50Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
appsmith	✅ Ready (Inspect)	Visit Preview	Dec 19, 2022 at 2:53AM (UTC)

SatishGandham

LGTM except that one minor change requested.

Good job with the PR description :)

SatishGandham · 2022-12-14T12:24:20Z

app/client/src/selectors/propertyPaneSelectors.tsx

-import { EVALUATION_PATH } from "utils/DynamicBindingUtils";
+import {
+  EVALUATION_PATH,
+  isPathADynamicProperty,


NIT. I don't think we need articles here. isPathDynamicProperty should be fine.

@SatishGandham
isPathADynamicTrigger and isPathADynamicProperty is being used in multiple places. Should I change it in this PR?

SatishGandham · 2022-12-14T12:30:18Z

@aswathkk , any performance numbers?

aswathkk · 2022-12-14T12:33:29Z

@SatishGandham I found the scripting time reduced from ~50ms to ~25ms after clicking on the JS toggle.

aswathkk · 2022-12-15T07:01:22Z

/ok-to-test sha=ab2b231

github-actions · 2022-12-15T07:01:53Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3701831117.
Workflow: Appsmith External Integration Test Workflow.
Commit: ab2b231.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3701831117_1

aswathkk · 2022-12-15T16:15:46Z

/ok-to-test sha=ab2b231

github-actions · 2022-12-15T16:16:20Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3705900418.
Workflow: Appsmith External Integration Test Workflow.
Commit: ab2b231.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3705900418_1

aswathkk · 2022-12-15T21:16:40Z

/perf-test sha=ab2b231

github-actions · 2022-12-15T21:17:11Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3708015282.
Workflow: Performance Tests Workflow.
Commit: ab2b231.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3708015282_1

aswathkk · 2022-12-19T02:45:55Z

/ok-to-test sha=824cdd1

github-actions · 2022-12-19T02:46:28Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3727945159.
Workflow: Appsmith External Integration Test Workflow.
Commit: 824cdd1.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3727945159_1

aswathkk · 2022-12-19T05:58:16Z

/perf-test sha=ab2b231

github-actions · 2022-12-19T05:58:42Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3728841549.
Workflow: Performance Tests Workflow.
Commit: ab2b231.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3728841549_1

aswathkk · 2022-12-19T07:07:59Z

/perf-test sha=ab2b231

github-actions · 2022-12-19T07:08:24Z

Tests running at: https://github.com/appsmithorg/appsmith/actions/runs/3729221059.
Workflow: Performance Tests Workflow.
Commit: ab2b231.
PR: 18905.
Perf tests will be available at https://app.appsmith.com/app/performance-infra-dashboard/pr-details-638dd7cd2913ba43778b915e?pr=18905&runId=3729221059_1

Aswath K added 3 commits December 12, 2022 17:47

fix: All property control renders when dynamicPropertyPathList changes

c835fa3

fix: Prevents re-render of ActionCreator

d78bb76

fix: All property control renders when dynamicTriggerPathList changes

e7285fc

vercel bot had a problem deploying to Preview December 13, 2022 02:32 Failure

fix: Type issue

e283504

appsmithorg deleted a comment from github-actions bot Dec 13, 2022

vercel bot deployed to Preview December 13, 2022 04:54 View deployment

Aswath K added 2 commits December 13, 2022 20:19

Merge branch 'release' into fix/pp-rendering

654cfe9

prevents rendering of actionCreator

9b741ed

vercel bot deployed to Preview December 13, 2022 18:56 View deployment

fix: Type issue with proxy-memoize

1a0cbc4

vercel bot had a problem deploying to Preview December 14, 2022 03:53 Failure

appsmithorg deleted a comment from github-actions bot Dec 14, 2022

fix jest module issue

7b52c7d

appsmithorg deleted a comment from github-actions bot Dec 14, 2022

vercel bot deployed to Preview December 14, 2022 04:36 View deployment

makes use fo utility fn rather than custom code

a801381

aswathkk marked this pull request as ready for review December 14, 2022 07:12

aswathkk requested a review from rahulramesha December 14, 2022 07:12

vercel bot deployed to Preview December 14, 2022 07:17 View deployment

aswathkk requested a review from SatishGandham December 14, 2022 07:20

appsmithorg deleted a comment from github-actions bot Dec 14, 2022

SatishGandham previously approved these changes Dec 14, 2022

View reviewed changes

Merge branch 'release' into fix/pp-rendering

f501a34

aswathkk removed the request for review from rahulramesha December 15, 2022 05:33

Aswath K added 3 commits December 15, 2022 12:21

Merge branch 'release' into fix/pp-rendering

b49f5ba

rename isPathADynamicProperty to isPathDynamicProperty

908fed5

rename isPathADynamicTrigger to isPathDynamicTrigger

ab2b231

aswathkk dismissed SatishGandham’s stale review via ab2b231 December 15, 2022 07:00

vercel bot deployed to Preview December 15, 2022 07:08 View deployment

Merge branch 'release' into fix/pp-rendering

824cdd1

vercel bot deployed to Preview December 19, 2022 02:53 View deployment

aswathkk enabled auto-merge (squash) December 19, 2022 04:45

sharat87 disabled auto-merge December 19, 2022 10:15

sharat87 merged commit f9e17f6 into release Dec 19, 2022

sharat87 deleted the fix/pp-rendering branch December 19, 2022 10:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: All property control renders when JS toggle button is clicked #18905

fix: All property control renders when JS toggle button is clicked #18905

aswathkk commented Dec 13, 2022 •

edited

vercel bot commented Dec 13, 2022 •

edited

SatishGandham left a comment

SatishGandham Dec 14, 2022

aswathkk Dec 14, 2022

SatishGandham commented Dec 14, 2022

aswathkk commented Dec 14, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

fix: All property control renders when JS toggle button is clicked #18905

fix: All property control renders when JS toggle button is clicked #18905

Conversation

aswathkk commented Dec 13, 2022 • edited

Description

Type of change

How Has This Been Tested?

Issues raised during DP testing

Checklist:

Dev activity

QA activity:

vercel bot commented Dec 13, 2022 • edited

SatishGandham left a comment

Choose a reason for hiding this comment

SatishGandham Dec 14, 2022

Choose a reason for hiding this comment

aswathkk Dec 14, 2022

Choose a reason for hiding this comment

SatishGandham commented Dec 14, 2022

aswathkk commented Dec 14, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 15, 2022

github-actions bot commented Dec 15, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

aswathkk commented Dec 19, 2022

github-actions bot commented Dec 19, 2022

aswathkk commented Dec 13, 2022 •

edited

vercel bot commented Dec 13, 2022 •

edited