fix: Updated drop down control memo usage (#11218)

* Stopped props drilling of eval state * Connect drop down to redux state * Added extra check to formcontrol memo function * Reduced modification of section at top * Stopped mutating the initial state * Created selector to get dynamic fetched values * <refactor> Added comments and refactors - Added key to the ES fragment - Cleaned drop down component from redundant code - Added comments * <refactor> Removed test files - Removed testing JSON configs * <fix> Added null check for form eval output - Added check to prevent null evalOutput in forms * <chore> Removed console error - Removed console error which is causing the vercel builds to fail
2022-02-26 22:41:38 +05:30 · 2022-02-26 22:41:38 +05:30 · fe2d625f5e
commit fe2d625f5e
parent a2240c7107
8 changed files with 123 additions and 143 deletions
--- a/app/client/src/components/formControls/BaseControl.tsx
+++ b/app/client/src/components/formControls/BaseControl.tsx
@ -1,10 +1,7 @@
 import { Component } from "react";
 import { ControlType } from "constants/PropertyControlConstants";
 import { InputType } from "components/constants";
-import {
-  ConditonalObject,
-  DynamicValues,
-} from "reducers/evaluationReducers/formEvaluationReducer";
+import { ConditonalObject } from "reducers/evaluationReducers/formEvaluationReducer";
 import { DropdownOption } from "components/ads/Dropdown";
 // eslint-disable-next-line @typescript-eslint/ban-types
 abstract class BaseControl<P extends ControlProps, S = {}> extends Component<
@ -77,7 +74,6 @@ export interface ControlData {
  identifier?: string;
  sectionName?: string;
  disabled?: boolean;
-  dynamicFetchedValues?: DynamicValues; // Object that holds the output of the dynamic fetched values
 }
 export type FormConfig = Omit<ControlData, "configProperty"> & {
  configProperty?: string;
--- a/app/client/src/components/formControls/DropDownControl.tsx
+++ b/app/client/src/components/formControls/DropDownControl.tsx
@ -9,7 +9,9 @@ import {
  WrappedFieldInputProps,
  WrappedFieldMetaProps,
 } from "redux-form";
-import { DynamicValues } from "reducers/evaluationReducers/formEvaluationReducer";
+import { connect } from "react-redux";
+import { AppState } from "reducers";
+import { getDynamicFetchedValues } from "selectors/formSelectors";

 const DropdownSelect = styled.div`
  font-size: 14px;
@ -27,23 +29,12 @@ class DropDownControl extends BaseControl<DropDownControlProps> {
      width = this.props.customStyles.width;
    }

-    // Options will be set dynamically if the config has fetchOptionsConditionally set to true
-    let options = this.props.options;
-    let isLoading = false;
-    if (
-      this.props.fetchOptionsCondtionally &&
-      !!this.props.dynamicFetchedValues
-    ) {
-      options = this.props.dynamicFetchedValues.data;
-      isLoading = this.props.dynamicFetchedValues.isLoading;
-    }
-
    return (
      <DropdownSelect data-cy={this.props.configProperty} style={{ width }}>
        <Field
          component={renderDropdown}
          name={this.props.configProperty}
-          props={{ ...this.props, width, isLoading, options }} // Passing options and isLoading in props allows the component to get the updated values
+          props={{ ...this.props, width }}
          type={this.props?.isMultiSelect ? "select-multiple" : undefined}
        />
      </DropdownSelect>
@ -55,39 +46,38 @@ class DropDownControl extends BaseControl<DropDownControlProps> {
  }
 }

-function renderDropdown(props: {
-  input?: WrappedFieldInputProps;
-  meta?: WrappedFieldMetaProps;
-  props: DropDownControlProps;
-  width: string;
-  formName: string;
-  isLoading?: boolean;
-  options: DropdownOption[];
-  disabled?: boolean;
-}): JSX.Element {
+function renderDropdown(
+  props: {
+    input?: WrappedFieldInputProps;
+    meta?: Partial<WrappedFieldMetaProps>;
+    width: string;
+  } & DropDownControlProps,
+): JSX.Element {
  let selectedValue = props.input?.value;
  if (_.isUndefined(props.input?.value)) {
-    selectedValue = props?.props?.initialValue;
+    selectedValue = props?.initialValue;
+  }
+  let options: DropdownOption[] = [];
+  let selectedOption = {};
+  if (typeof props.options === "object" && Array.isArray(props.options)) {
+    options = props.options;
+    selectedOption =
+      options.find(
+        (option: DropdownOption) => option.value === selectedValue,
+      ) || {};
  }
-
-  const selectedOption =
-    props.options.find(
-      (option: DropdownOption) => option.value === selectedValue,
-    ) || {};
  return (
    <Dropdown
      boundary="window"
      disabled={props.disabled}
      dontUsePortal={false}
      dropdownMaxHeight="250px"
-      errorMsg={props.props?.errorText}
-      helperText={props.props?.info}
      isLoading={props.isLoading}
-      isMultiSelect={props?.props?.isMultiSelect}
+      isMultiSelect={props?.isMultiSelect}
      onSelect={props.input?.onChange}
      optionWidth={props.width}
-      options={props.options}
-      placeholder={props.props?.placeholderText}
+      options={options}
+      placeholder={props?.placeholderText}
      selected={selectedOption}
      showLabelOnly
      width={props.width}
@ -103,7 +93,36 @@ export interface DropDownControlProps extends ControlProps {
  isMultiSelect?: boolean;
  isSearchable?: boolean;
  fetchOptionsCondtionally?: boolean;
-  dynamicFetchedValues?: DynamicValues;
+  isLoading: boolean;
 }

-export default DropDownControl;
+const mapStateToProps = (
+  state: AppState,
+  ownProps: DropDownControlProps,
+): { isLoading: boolean; options: DropdownOption[] } => {
+  // Added default options to prevent error when options is undefined
+  let isLoading = false;
+  let options: DropdownOption[] = ownProps.fetchOptionsCondtionally
+    ? []
+    : ownProps.options;
+
+  try {
+    if (ownProps.fetchOptionsCondtionally) {
+      const dynamicFetchedValues = getDynamicFetchedValues(
+        state,
+        ownProps.configProperty,
+      );
+      isLoading = dynamicFetchedValues.isLoading;
+      options = dynamicFetchedValues.data;
+    }
+    return { isLoading, options };
+  } catch (e) {
+    return {
+      isLoading,
+      options,
+    };
+  }
+};
+
+// Connecting this componenet to the state to allow for dynamic fetching of options to be updated.
+export default connect(mapStateToProps)(DropDownControl);
--- a/app/client/src/components/formControls/EntitySelectorControl.tsx
+++ b/app/client/src/components/formControls/EntitySelectorControl.tsx
@ -1,11 +1,9 @@
 import React from "react";
 import FormControl from "pages/Editor/FormControl";
 import styled from "styled-components";
-import FormLabel from "components/editorComponents/FormLabel";
 import { ControlProps } from "./BaseControl";
 import { Colors } from "constants/Colors";
 import Icon, { IconSize } from "components/ads/Icon";
-import { getBindingOrConfigPathsForEntitySelectorControl } from "entities/Action/actionProperties";
 import { allowedControlTypes } from "components/formControls/utils";

 const dropDownFieldConfig: any = {
@ -39,15 +37,6 @@ const EntitySelectorContainer = styled.div`
  justify-content: space-between;
 `;

-export const StyledBottomLabel = styled(FormLabel)`
-  margin-top: 5px;
-  margin-left: 5px;
-  font-weight: 400;
-  font-size: 12px;
-  color: ${Colors.GREY_7};
-  line-height: 16px;
-`;
-
 function EntitySelectorComponent(props: any) {
  const { configProperty, schema } = props;

@ -61,25 +50,20 @@ function EntitySelectorComponent(props: any) {
  };

  return (
-    <EntitySelectorContainer>
+    <EntitySelectorContainer key={`ES_${configProperty}`}>
      {schema &&
        schema.length > 0 &&
        schema.map((singleSchema: any, index: number) => {
-          const columnPath = getBindingOrConfigPathsForEntitySelectorControl(
-            configProperty,
-            index,
-          );
          return (
            allowedControlTypes.includes(singleSchema.controlType) && (
-              <>
+              <React.Fragment key={`ES_FRAG_${singleSchema.configProperty}`}>
                {singleSchema.controlType === "DROP_DOWN" ? (
                  <FormControl
                    config={{
                      ...dropDownFieldConfig,
                      ...singleSchema,
                      customStyles,
-                      configProperty: columnPath,
-                      key: columnPath,
+                      key: `ES_${singleSchema.configProperty}`,
                    }}
                    formName={props.formName}
                  />
@ -89,19 +73,19 @@ function EntitySelectorComponent(props: any) {
                      ...inputFieldConfig,
                      ...singleSchema,
                      customStyles,
-                      configProperty: columnPath,
-                      key: columnPath,
+                      key: `ES_${singleSchema.configProperty}`,
                    }}
                    formName={props.formName}
                  />
                )}
                {index < schema.length - 1 && (
                  <CenteredIcon
+                    key={`ES_ICON_${configProperty}`}
                    name="double-arrow-right"
                    size={IconSize.SMALL}
                  />
                )}
-              </>
+              </React.Fragment>
            )
          );
        })}
@ -109,6 +93,8 @@ function EntitySelectorComponent(props: any) {
  );
 }

+// This is a wrapper component that just encapsulated the children dropdown and dynamic text
+// components & changes their appearance
 export default function EntitySelectorControl(
  props: EntitySelectorControlProps,
 ) {
@ -122,7 +108,7 @@ export default function EntitySelectorControl(
    <EntitySelectorComponent
      configProperty={configProperty}
      formName={formName}
-      key={configProperty}
+      key={`ES_PARENT_${configProperty}`}
      name={configProperty}
      schema={schema}
    />
--- a/app/client/src/pages/Editor/FormControl.tsx
+++ b/app/client/src/pages/Editor/FormControl.tsx
@ -51,7 +51,7 @@ function FormControl(props: FormControlProps) {
        props.formName,
        props?.multipleConfig,
      ),
-    [],
+    [props],
  );

  if (hidden) return null;
@ -133,7 +133,13 @@ function FormConfig(props: FormConfigProps) {
  );
 }

-export default memo(FormControl);
+// Updated the memo function to allow for disabled props to be compared
+export default memo(FormControl, (prevProps, nextProps) => {
+  return (
+    prevProps === nextProps &&
+    prevProps.config.disabled === nextProps.config.disabled
+  );
+});

 function renderFormConfigTop(props: { config: ControlProps }) {
  const {
--- a/app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx
+++ b/app/client/src/pages/Editor/QueryEditor/EditorJSONtoForm.tsx
@ -80,7 +80,6 @@ import Spinner from "components/ads/Spinner";
 import {
  ConditionalOutput,
  FormEvalOutput,
-  DynamicValues,
 } from "reducers/evaluationReducers/formEvaluationReducer";

 const QueryFormContainer = styled.form`
@ -598,6 +597,7 @@ export function EditorJSONtoForm(props: Props) {
    }
  };

+  // Extract the output of conditionals attached to the form from the state
  const extractConditionalOutput = (section: any): ConditionalOutput => {
    let conditionalOutput: ConditionalOutput = {};
    if (
@ -649,67 +649,35 @@ export function EditorJSONtoForm(props: Props) {
  };

  // Function to modify the section config based on the output of evaluations
-  const modifySectionConfig = (
-    section: any,
-    enabled: boolean,
-    dynamicFetchedValues: DynamicValues | undefined,
-  ): any => {
+  const modifySectionConfig = (section: any, enabled: boolean): any => {
    if (!enabled) {
      section.disabled = true;
    } else {
      section.disabled = false;
    }
-    if (!!dynamicFetchedValues) {
-      section.dynamicFetchedValues = dynamicFetchedValues;
-    }

    return section;
  };

-  // Function to extract the object for dynamicValues if it is there in the evaluation state
-  const extractDynamicValuesIfPresent = (
-    conditionalOutput: ConditionalOutput,
-  ) => {
-    // By default, the section is enabled. This is to allow for the case where no conditional is provided.
-    // The evaluation state disables the section if the condition is not met. (Checkout formEval.ts)
-    let dynamicFetchedValues: DynamicValues | undefined;
-    if (conditionalOutput.hasOwnProperty("fetchDynamicValues")) {
-      dynamicFetchedValues = conditionalOutput.fetchDynamicValues;
-    }
-    return dynamicFetchedValues;
-  };
-
  // Render function to render the V2 of form editor type (UQI)
  // Section argument is a nested config object, this function recursively renders the UI based on the config
  const renderEachConfigV2 = (formName: string, section: any, idx: number) => {
    let enabled = true;
-    let dynamicFetchedValues: DynamicValues | undefined;
    if (!!section) {
+      // If the section is a nested component, recursively check for conditional statements
      if ("schema" in section && section.schema.length > 0) {
-        section.schema.forEach((subSection: any, index: number) => {
-          const configPropertyOfSubSection = `${
-            section.configProperty
-          }.column_${index + 1}`;
+        section.schema.forEach((subSection: any) => {
          const conditionalOutput = extractConditionalOutput({
            ...subSection,
-            configProperty: configPropertyOfSubSection,
          });
          enabled = checkIfSectionIsEnabled(conditionalOutput);
-          dynamicFetchedValues = extractDynamicValuesIfPresent(
-            conditionalOutput,
-          );
-          subSection = modifySectionConfig(
-            subSection,
-            enabled,
-            dynamicFetchedValues,
-          );
+          subSection = modifySectionConfig(subSection, enabled);
        });
      }
      // If the component is not allowed to render, return null
      const conditionalOutput = extractConditionalOutput(section);
      if (!checkIfSectionCanRender(conditionalOutput)) return null;
      enabled = checkIfSectionIsEnabled(conditionalOutput);
-      dynamicFetchedValues = extractDynamicValuesIfPresent(conditionalOutput);
    }
    if (section.hasOwnProperty("controlType")) {
      // If component is type section, render it's children
@ -723,11 +691,7 @@ export function EditorJSONtoForm(props: Props) {
      }
      try {
        const { configProperty } = section;
-        const modifiedSection = modifySectionConfig(
-          section,
-          enabled,
-          dynamicFetchedValues,
-        );
+        const modifiedSection = modifySectionConfig(section, enabled);
        return (
          <FieldWrapper key={`${configProperty}_${idx}`}>
            <FormControl config={modifiedSection} formName={formName} />
--- a/app/client/src/sagas/FormEvaluationSaga.ts
+++ b/app/client/src/sagas/FormEvaluationSaga.ts
@ -87,16 +87,18 @@ function* setFormEvaluationSagaAsync(
        // Once all the actions are done, extract the actions that need to be fetched dynamically
        const formId = action.payload.formId;
        const evalOutput = workerResponse[formId];
-        const queueOfValuesToBeFetched = extractQueueOfValuesToBeFetched(
-          evalOutput,
-        );
-        // Pass the queue to the saga to fetch the dynamic values
-        yield call(
-          fetchDynamicValuesSaga,
-          queueOfValuesToBeFetched,
-          formId,
-          evalOutput,
-        );
+        if (!!evalOutput && typeof evalOutput === "object") {
+          const queueOfValuesToBeFetched = extractQueueOfValuesToBeFetched(
+            evalOutput,
+          );
+          // Pass the queue to the saga to fetch the dynamic values
+          yield call(
+            fetchDynamicValuesSaga,
+            queueOfValuesToBeFetched,
+            formId,
+            evalOutput,
+          );
+        }
      }
    } catch (e) {
      log.error(e);
@ -112,11 +114,10 @@ function* fetchDynamicValuesSaga(
  evalOutput: FormEvalOutput,
 ) {
  for (const key of Object.keys(queueOfValuesToBeFetched)) {
-    evalOutput = yield call(
+    evalOutput[key].fetchDynamicValues = yield call(
      fetchDynamicValueSaga,
      queueOfValuesToBeFetched[key],
-      key,
-      evalOutput,
+      Object.assign({}, evalOutput[key].fetchDynamicValues as DynamicValues),
    );
  }
  // Set the values to the state once all values are fetched
@ -128,34 +129,31 @@ function* fetchDynamicValuesSaga(

 function* fetchDynamicValueSaga(
  value: ConditionalOutput,
-  key: string,
-  evalOutput: FormEvalOutput,
+  dynamicFetchedValues: DynamicValues,
 ) {
  try {
    const { config } = value.fetchDynamicValues as DynamicValues;
    const { url } = config;

-    (evalOutput[key].fetchDynamicValues as DynamicValues).hasStarted = true;
+    dynamicFetchedValues.hasStarted = true;

    // Call the API to fetch the dynamic values
    const response = yield call(PluginsApi.fetchDynamicFormValues, url);
-    (evalOutput[key].fetchDynamicValues as DynamicValues).isLoading = false;
+    dynamicFetchedValues.isLoading = false;
    if (!!response && response instanceof Array) {
-      (evalOutput[key].fetchDynamicValues as DynamicValues).data = response;
-      (evalOutput[key]
-        .fetchDynamicValues as DynamicValues).hasFetchFailed = false;
+      dynamicFetchedValues.data = response;
+      dynamicFetchedValues.hasFetchFailed = false;
    } else {
-      (evalOutput[key]
-        .fetchDynamicValues as DynamicValues).hasFetchFailed = true;
-      (evalOutput[key].fetchDynamicValues as DynamicValues).data = [];
+      dynamicFetchedValues.hasFetchFailed = true;
+      dynamicFetchedValues.data = [];
    }
  } catch (e) {
    log.error(e);
-    (evalOutput[key].fetchDynamicValues as DynamicValues).hasFetchFailed = true;
-    (evalOutput[key].fetchDynamicValues as DynamicValues).isLoading = false;
-    (evalOutput[key].fetchDynamicValues as DynamicValues).data = [];
+    dynamicFetchedValues.hasFetchFailed = true;
+    dynamicFetchedValues.isLoading = false;
+    dynamicFetchedValues.data = [];
  }
-  return evalOutput;
+  return dynamicFetchedValues;
 }

 function* formEvaluationChangeListenerSaga() {
--- a/app/client/src/selectors/formSelectors.ts
+++ b/app/client/src/selectors/formSelectors.ts
@ -1,13 +1,17 @@
 import { getFormValues, isValid, getFormInitialValues } from "redux-form";
 import { AppState } from "reducers";
 import { ActionData } from "reducers/entityReducers/actionsReducer";
-import { FormEvaluationState } from "reducers/evaluationReducers/formEvaluationReducer";
+import {
+  DynamicValues,
+  FormEvaluationState,
+} from "reducers/evaluationReducers/formEvaluationReducer";
 import { createSelector } from "reselect";
-import _ from "lodash";
+import { replace } from "lodash";
 import { getDataTree } from "./dataTreeSelectors";
 import { DataTree } from "entities/DataTree/dataTreeFactory";
 import { Action } from "entities/Action";
 import { EvaluationError } from "utils/DynamicBindingUtils";
+import { getActionIdFromURL } from "pages/Editor/Explorer/helpers";

 type GetFormData = (
  state: AppState,
@ -30,6 +34,16 @@ export const getApiName = (state: AppState, id: string) => {
 export const getFormEvaluationState = (state: AppState): FormEvaluationState =>
  state.evaluations.formEvaluation;

+// Selector to return the fetched values of the form components, only called for components that
+// have the fetchOptionsDynamically option set to true
+export const getDynamicFetchedValues = (
+  state: AppState,
+  configProperty: string,
+): DynamicValues =>
+  state.evaluations.formEvaluation[getActionIdFromURL() as string][
+    configProperty
+  ].fetchDynamicValues as DynamicValues;
+
 type ConfigErrorProps = { configProperty: string; formName: string };

 export const getConfigErrors = createSelector(
@ -53,7 +67,7 @@ export const getConfigErrors = createSelector(
        const actionError = action && action?.__evaluation__?.errors;

        // get the configProperty for this form control and format it to resemble the format used in the action details errors object.
-        const formattedConfig = _.replace(
+        const formattedConfig = replace(
          configProperty,
          "actionConfiguration",
          "config",
--- a/app/client/src/workers/formEval.ts
+++ b/app/client/src/workers/formEval.ts
@ -85,11 +85,8 @@ const generateInitialEvalState = (formConfig: FormConfig) => {
    );

  if ("schema" in formConfig && !!formConfig.schema)
-    formConfig.schema.forEach((config: FormConfig, index: number) =>
-      generateInitialEvalState({
-        ...config,
-        configProperty: `${formConfig.configProperty}.column_${index + 1}`,
-      }),
+    formConfig.schema.forEach((config: FormConfig) =>
+      generateInitialEvalState({ ...config }),
    );
 };