Skip to content

chore: Refactor derived properties #37390

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jsartisan wants to merge 3 commits into from
Closed

chore: Refactor derived properties #37390

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e21d65
refactor derived properties
jsartisan Nov 14, 2024
5c991ba
refactor derived properties
jsartisan Nov 14, 2024
ef80ed0
refactor derived properties
jsartisan Nov 14, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { WidgetState } from "widgets/BaseWidget";
import { EventType } from "constants/AppsmithActionConstants/ActionConstants";

import { CurrencyInputComponent } from "../component";
import derivedProperties from "./parsedDerivedProperties";

import {
formatCurrencyNumber,
limitDecimalValue,
Expand All @@ -19,7 +19,9 @@ import {
import {
getLocaleDecimalSeperator,
getLocaleThousandSeparator,
parseDerivedProperties,
} from "widgets/WidgetUtils";
import derivedPropertyFns from "./derived";
import type { SetterConfig, Stylesheet } from "entities/AppTheming";
import type {
AnvilConfig,
Expand Down Expand Up @@ -120,8 +122,10 @@ class WDSCurrencyInputWidget extends WDSBaseInputWidget<
}

static getDerivedPropertiesMap() {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return {
isValid: `{{(() => {${derivedProperties.isValid}})()}}`,
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
};
Comment on lines +125 to 129
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Derived properties implementation is incomplete

The implementation is missing the derived.ts file that should contain the isValid property function. This file needs to be created to complete the derived properties migration.

  • Create derived.ts in the app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/ directory
  • Implement the isValid derived property function
🔗 Analysis chain

Verify derived properties migration

The implementation looks good, using the new parseDerivedProperties utility. Let's verify the derived properties implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Verify the derived properties implementation
# Check if the derivedPropertyFns module exports the expected properties

# Look for isValid implementation in derived.ts
fd -e ts -e tsx -x grep -l "isValid.*export" {}

Length of output: 2514

Script:

#!/bin/bash
# Let's try a different approach to find the derived properties implementation
# First, let's find the derived.ts file in the WDSCurrencyInputWidget directory
fd -t f "derived.ts" app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget

# Then let's examine its content if found
fd -t f "derived.ts" app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget -x cat {}

# Also check for any other files that might contain the derived properties implementation
rg -l "derivedPropertyFns" app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget

Length of output: 353

Script:

#!/bin/bash
# Let's examine the content of index.tsx to understand the derivedPropertyFns implementation
cat app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/index.tsx

# Also search for any parseDerivedProperties utility implementation
rg -g '!*.d.ts' -l "parseDerivedProperties" app/client/src/modules/ui-builder/ui/wds

Length of output: 11154

Script:

#!/bin/bash
# Let's check the derived.ts file content to verify the isValid implementation
cat app/client/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/derived.ts

# Also check the parseDerivedProperties utility implementation
rg -g '!*.d.ts' "export.*parseDerivedProperties" -A 5

Length of output: 767

}

Expand Down
40 changes: 0 additions & 40 deletions ...nt/src/modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/parsedDerivedProperties.ts
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions app/client/src/modules/ui-builder/ui/wds/WDSInputWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ import { mergeWidgetConfig } from "utils/helpers";
import { parseText, validateInput } from "./helper";
import type { WidgetState } from "widgets/BaseWidget";
import type { SetterConfig } from "entities/AppTheming";
import derivedProperties from "./parsedDerivedProperties";
import derivedPropertyFns from "./derived";
import { parseDerivedProperties } from "widgets/WidgetUtils";
import { WDSBaseInputWidget } from "../../WDSBaseInputWidget";
import type { DerivedPropertiesMap } from "WidgetProvider/factory";
import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
Expand Down Expand Up @@ -52,8 +53,10 @@ class WDSInputWidget extends WDSBaseInputWidget<InputWidgetProps, WidgetState> {
}

static getDerivedPropertiesMap(): DerivedPropertiesMap {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return merge(super.getDerivedPropertiesMap(), {
isValid: `{{(() => {${derivedProperties.isValid}})()}}`,
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
});
}

Expand Down
40 changes: 0 additions & 40 deletions app/client/src/modules/ui-builder/ui/wds/WDSInputWidget/widget/parsedDerivedProperties.ts
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,8 @@ import { WDSBaseInputWidget } from "modules/ui-builder/ui/wds/WDSBaseInputWidget
import { AsYouType, parseIncompletePhoneNumber } from "libphonenumber-js";
import { EventType } from "constants/AppsmithActionConstants/ActionConstants";
import type { KeyDownEvent } from "modules/ui-builder/ui/wds/WDSBaseInputWidget/component/types";
import derivedProperties from "modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/parsedDerivedProperties";
import derivedPropertyFns from "modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/derived";
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix incorrect import path for derivedPropertyFns

The derivedPropertyFns import appears to be from WDSCurrencyInputWidget instead of the current widget's path.

Apply this fix:

-import derivedPropertyFns from "modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/derived";
+import derivedPropertyFns from "./derived";
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
import derivedPropertyFns from "modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/derived";
import derivedPropertyFns from "./derived";

import { parseDerivedProperties } from "widgets/WidgetUtils";

import * as config from "../config";
import { PhoneInputComponent } from "../component";
Expand Down Expand Up @@ -107,8 +108,10 @@ class WDSPhoneInputWidget extends WDSBaseInputWidget<
}

static getDerivedPropertiesMap(): DerivedPropertiesMap {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return {
isValid: `{{(() => {${derivedProperties.isValid}})()}}`,
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
Comment on lines +111 to +114
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

Potential issue with derived properties import

The widget is importing derived properties from WDSCurrencyInputWidget instead of having its own implementation:

import derivedPropertyFns from "modules/ui-builder/ui/wds/WDSCurrencyInputWidget/widget/derived";

This appears to be incorrect as phone input validation would differ from currency input validation.

🔗 Analysis chain

Verify complete migration of derived properties

Let's ensure no derived properties were missed during the migration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining old derived properties implementation
rg -l "getDerivedPropertiesMap.*derivedProperties" "app/client/src/modules/ui-builder/ui/wds"

# Search for consistency in new implementation
rg -l "getDerivedPropertiesMap.*parseDerivedProperties" "app/client/src/modules/ui-builder/ui/wds"

Length of output: 192

Script:

#!/bin/bash
# Search for derived properties in the WDSPhoneInputWidget directory
rg -A 5 "derivedProperties|parseDerivedProperties" "app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget"

# Search for getDerivedPropertiesMap implementation
ast-grep --pattern 'getDerivedPropertiesMap() {
  $$$
}'

Length of output: 1550

Script:

#!/bin/bash
# Search for all derived properties in the widget
ast-grep --pattern 'const derivedPropertyFns = {
  $$$
}'

# Check for any other derived properties related functions
rg -A 3 "getDerivedProperties|derivedPropertyFns" "app/client/src/modules/ui-builder/ui/wds/WDSPhoneInputWidget"

Length of output: 1297

};
}

Expand Down
30 changes: 16 additions & 14 deletions app/client/src/modules/ui-builder/ui/wds/WDSTableWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@ import {
PaginationDirection,
TABLE_COLUMN_ORDER_KEY,
} from "../constants";
import derivedProperties from "./parseDerivedProperties";
import {
deleteLocalTableColumnOrderByWidgetId,
generateLocalNewColumnOrderFromStickyValue,
Expand All @@ -61,7 +60,7 @@ import {
} from "./utilities";
import type { BatchPropertyUpdatePayload } from "actions/controlActions";
import equal from "fast-deep-equal/es6";
import { sanitizeKey } from "widgets/WidgetUtils";
import { parseDerivedProperties, sanitizeKey } from "widgets/WidgetUtils";
import {
PlainTextCell,
URLCell,
Expand All @@ -82,6 +81,7 @@ import * as config from "../config";
import { getAnvilWidgetDOMId } from "layoutSystems/common/utils/LayoutElementPositionsObserver/utils";
import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer";
import { klonaRegularWithTelemetry } from "utils/helpers";
import derivedPropertyFns from "./derived";

const ReactTableComponent = lazy(async () =>
retryPromise(async () => import("../component")),
Expand Down Expand Up @@ -194,20 +194,22 @@ export class WDSTableWidget extends BaseWidget<TableWidgetProps, WidgetState> {
}

static getDerivedPropertiesMap() {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return {
selectedRow: `{{(()=>{${derivedProperties.getSelectedRow}})()}}`,
triggeredRow: `{{(()=>{${derivedProperties.getTriggeredRow}})()}}`,
selectedRows: `{{(()=>{${derivedProperties.getSelectedRows}})()}}`,
selectedRow: `{{(()=>{${parsedDerivedProperties.getSelectedRow}})()}}`,
triggeredRow: `{{(()=>{${parsedDerivedProperties.getTriggeredRow}})()}}`,
selectedRows: `{{(()=>{${parsedDerivedProperties.getSelectedRows}})()}}`,
triggerRowSelection: "{{!!this.onRowSelected}}",
processedTableData: `{{(()=>{${derivedProperties.getProcessedTableData}})()}}`,
orderedTableColumns: `{{(()=>{${derivedProperties.getOrderedTableColumns}})()}}`,
filteredTableData: `{{(()=>{ ${derivedProperties.getFilteredTableData}})()}}`,
updatedRows: `{{(()=>{ ${derivedProperties.getUpdatedRows}})()}}`,
updatedRowIndices: `{{(()=>{ ${derivedProperties.getUpdatedRowIndices}})()}}`,
updatedRow: `{{(()=>{ ${derivedProperties.getUpdatedRow}})()}}`,
pageOffset: `{{(()=>{${derivedProperties.getPageOffset}})()}}`,
isEditableCellsValid: `{{(()=>{ ${derivedProperties.getEditableCellValidity}})()}}`,
tableHeaders: `{{(()=>{${derivedProperties.getTableHeaders}})()}}`,
processedTableData: `{{(()=>{${parsedDerivedProperties.getProcessedTableData}})()}}`,
Copy link
Contributor

@coderabbitai coderabbitai bot Nov 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix: Cannot access this.onRowSelected in a static method

The reference to this.onRowSelected inside the static method getDerivedPropertiesMap is invalid since this is undefined in static context.

Apply this diff to fix the issue:

-static getDerivedPropertiesMap() {
+getDerivedPropertiesMap() {
...
  triggerRowSelection: "{{!!this.onRowSelected}}",
...
}

Consider moving triggerRowSelection to an instance method or refactoring the code to avoid using this in a static context.

Committable suggestion skipped: line range outside the PR's diff.

orderedTableColumns: `{{(()=>{${parsedDerivedProperties.getOrderedTableColumns}})()}}`,
filteredTableData: `{{(()=>{ ${parsedDerivedProperties.getFilteredTableData}})()}}`,
updatedRows: `{{(()=>{ ${parsedDerivedProperties.getUpdatedRows}})()}}`,
updatedRowIndices: `{{(()=>{ ${parsedDerivedProperties.getUpdatedRowIndices}})()}}`,
updatedRow: `{{(()=>{ ${parsedDerivedProperties.getUpdatedRow}})()}}`,
pageOffset: `{{(()=>{${parsedDerivedProperties.getPageOffset}})()}}`,
isEditableCellsValid: `{{(()=>{ ${parsedDerivedProperties.getEditableCellValidity}})()}}`,
tableHeaders: `{{(()=>{${parsedDerivedProperties.getTableHeaders}})()}}`,
};
}

Expand Down
41 changes: 0 additions & 41 deletions app/client/src/modules/ui-builder/ui/wds/WDSTableWidget/widget/parseDerivedProperties.ts
View file
Open in desktop

This file was deleted.

9 changes: 6 additions & 3 deletions app/client/src/widgets/CurrencyInputWidget/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
} from "../component/CurrencyCodeDropdown";
import { AutocompleteDataType } from "utils/autocomplete/AutocompleteDataType";
import _ from "lodash";
import derivedProperties from "./parsedDerivedProperties";
import derivedPropertyFns from "./derived";
import { parseDerivedProperties } from "widgets/WidgetUtils";
import BaseInputWidget from "widgets/BaseInputWidget";
import type { BaseInputWidgetProps } from "widgets/BaseInputWidget/widget";
import * as Sentry from "@sentry/react";
Expand Down Expand Up @@ -422,9 +423,11 @@ class CurrencyInputWidget extends BaseInputWidget<
}

static getDerivedPropertiesMap(): DerivedPropertiesMap {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return {
isValid: `{{(()=>{${derivedProperties.isValid}})()}}`,
value: `{{(()=>{${derivedProperties.value}})()}}`,
isValid: `{{(()=>{${parsedDerivedProperties.isValid}})()}}`,
value: `{{(()=>{${parsedDerivedProperties.value}})()}}`,
};
}

Expand Down
40 changes: 0 additions & 40 deletions app/client/src/widgets/CurrencyInputWidget/widget/parsedDerivedProperties.ts
View file
Open in desktop

This file was deleted.

7 changes: 5 additions & 2 deletions app/client/src/widgets/DatePickerWidget2/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,8 @@ import {
import type { DatePickerType } from "../constants";
import { TimePrecision } from "../constants";
import { DateFormatOptions } from "./constants";
import derivedProperties from "./parseDerivedProperties";
import derivedPropertyFns from "./derived";
import { parseDerivedProperties } from "widgets/WidgetUtils";
import { isAutoLayout } from "layoutSystems/autolayout/utils/flexWidgetUtils";
import type {
AnvilConfig,
Expand Down Expand Up @@ -632,8 +633,10 @@ class DatePickerWidget extends BaseWidget<DatePickerWidget2Props, WidgetState> {
}

static getDerivedPropertiesMap(): DerivedPropertiesMap {
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);

return {
isValid: `{{(()=>{${derivedProperties.isValidDate}})()}}`,
isValid: `{{(()=>{${parsedDerivedProperties.isValidDate}})()}}`,
selectedDate: `{{ this.value ? moment(this.value).toISOString() : "" }}`,
formattedDate: `{{ this.value ? moment(this.value).format(this.dateFormat) : "" }}`,
};
Expand Down
40 changes: 0 additions & 40 deletions app/client/src/widgets/DatePickerWidget2/widget/parseDerivedProperties.ts
View file
Open in desktop

This file was deleted.

Loading