Skip to content
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

Jump to bottom

chore: Refactor derived properties #37390

Draft
wants to merge 3 commits into
base: release
Choose a base branch
from
Draft

chore: Refactor derived properties #37390

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f088f14
refactor derived properties
jsartisan Nov 14, 2024
3347af1
refactor derived properties
jsartisan Nov 14, 2024
e895c38
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

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.

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

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.

7 changes: 5 additions & 2 deletions app/client/src/widgets/InputWidgetV2/widget/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,8 @@ import {
import type { InputComponentProps } from "../component";
import InputComponent from "../component";
import { getParsedText, isInputTypeEmailOrPassword } from "./Utilities";
import derivedProperties from "./parsedDerivedProperties";
import derivedPropertyFns from "./derived";
import { parseDerivedProperties } from "widgets/WidgetUtils";

import IconSVG from "../icon.svg";
import ThumbnailSVG from "../thumbnail.svg";
Expand Down Expand Up @@ -640,8 +641,10 @@ class InputWidget extends BaseInputWidget<InputWidgetProps, WidgetState> {
}

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

return merge(super.getDerivedPropertiesMap(), {
isValid: `{{(() => {${derivedProperties.isValid}})()}}`,
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
});
Comment on lines +644 to 648
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Replace super with class name in static context

Using super in a static context can be confusing. Consider using the class name instead.

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

-   return merge(super.getDerivedPropertiesMap(), {
+   return merge(BaseInputWidget.getDerivedPropertiesMap(), {
      isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
    });
  }
📝 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
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);
return merge(super.getDerivedPropertiesMap(), {
isValid: `{{(() => {${derivedProperties.isValid}})()}}`,
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
});
const parsedDerivedProperties = parseDerivedProperties(derivedPropertyFns);
return merge(BaseInputWidget.getDerivedPropertiesMap(), {
isValid: `{{(() => {${parsedDerivedProperties.isValid}})()}}`,
});
🧰 Tools
🪛 Biome

[error] 646-646: Using super in a static context can be confusing.

super refers to a parent class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

}

Expand Down
Loading
Loading