Skip to content

chore: bypass immer for first evaluation, fixed cloneDeep issue and using mutative instead of immer #38993

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

Merged
vsvamsi1 merged 1 commit into from
Feb 6, 2025
Merged

chore: bypass immer for first evaluation, fixed cloneDeep issue and using mutative instead of immer #38993

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion app/client/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,7 +140,6 @@
"fusioncharts": "^3.18.0",
"graphql": "^16.8.1",
"history": "^4.10.1",
"immer": "^9.0.6",
"interweave": "^12.7.2",
"interweave-autolink": "^4.4.2",
"js-regex-pl": "^1.0.1",
Expand All @@ -161,6 +160,7 @@
"mixpanel-browser": "^2.55.1",
"moment": "2.29.4",
"moment-timezone": "^0.5.35",
"mutative": "^1.1.0",
"nanoid": "^2.0.4",
"node-forge": "^1.3.0",
"object-hash": "^3.0.0",
Expand Down Expand Up @@ -331,6 +331,7 @@
"eslint-plugin-testing-library": "^6.2.0",
"factory.ts": "^0.5.1",
"husky": "^8.0.0",
"immer": "^9.0.6",
Copy link
Contributor

@rajatagrawal rajatagrawal Feb 6, 2025

Choose a reason for hiding this comment

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

@vsvamsi1 Is immer still required in dependencies ?

Copy link
Contributor Author

@vsvamsi1 vsvamsi1 Feb 6, 2025

Choose a reason for hiding this comment

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

I have left it in the devDependendices not in the dependencies because there were cypress testcases using it. This won't show up on the client prod build. I tried substituting them in the cypress testcases with mutative, however im seeing some errors accessing the mutative packages in cypress commands. I will reach out to @sagar-qa007 to help resolve this and remove them completely. This part of it can be resolved in a separate PR.

Copy link
Contributor

@rajatagrawal rajatagrawal Feb 6, 2025

Choose a reason for hiding this comment

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

Sounds good. We can merge the changes for now. I have also added a backlog item for it here

What kind of error are we facing ?

Copy link
Contributor Author

@vsvamsi1 vsvamsi1 Feb 6, 2025

Choose a reason for hiding this comment

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

The mutative updates were getting ignored and all feature flag interception logic was not working. So all test cases are failing prematurely at the sidebar assertion itself. I thought it was headless chrome running these test cases does not support proxy, however the reducer updates using mutative were working. It seems specific cypress commands.
cc @sagar-qa007

"jest": "^29.6.1",
"jest-canvas-mock": "^2.3.1",
"jest-environment-jsdom": "^29.6.1",
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/WidgetProvider/factory/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import {
import type { RegisteredWidgetFeatures } from "../../utils/WidgetFeatures";
import type { SetterConfig } from "entities/AppTheming";
import { freeze, memoize } from "./decorators";
import produce from "immer";
import { create } from "mutative";
import type { CanvasWidgetsReduxState } from "reducers/entityReducers/canvasWidgetsReducer";
import type {
CopiedWidgetData,
Expand Down Expand Up @@ -418,7 +418,7 @@ class WidgetFactory {

if (dynamicProperties && dynamicProperties.length) {
addPropertyConfigIds(dynamicProperties, false);
section = produce(section, (draft) => {
section = create(section, (draft) => {
draft.children = [...dynamicProperties, ...section.children];
});
}
Expand Down
8 changes: 4 additions & 4 deletions app/client/src/ce/pages/Editor/Explorer/hooks.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type { Datasource } from "entities/Datasource";
import { isStoredDatasource } from "entities/Action";
import type { WidgetProps } from "widgets/BaseWidget";
import log from "loglevel";
import produce from "immer";
import { create } from "mutative";
import type { CanvasStructure } from "reducers/uiReducers/pageCanvasStructureReducer";
import { getActions, getDatasources } from "ee/selectors/entitiesSelector";
import type { ActionData } from "ee/reducers/entityReducers/actionsReducer";
Expand Down Expand Up @@ -184,7 +184,7 @@ export const useActions = (searchKeyword?: string) => {
return useMemo(() => {
if (searchKeyword) {
const start = performance.now();
const filteredActions = produce(actions, (draft) => {
const filteredActions = create(actions, (draft) => {
for (const [key, value] of Object.entries(draft)) {
if (pageIds.includes(key)) {
draft[key] = value;
Expand Down Expand Up @@ -225,7 +225,7 @@ export const useWidgets = (searchKeyword?: string) => {
return useMemo(() => {
if (searchKeyword && pageCanvasStructures) {
const start = performance.now();
const filteredDSLs = produce(pageCanvasStructures, (draft) => {
const filteredDSLs = create(pageCanvasStructures, (draft) => {
for (const [key, value] of Object.entries(draft)) {
if (pageIds.includes(key)) {
draft[key] = value;
Expand Down Expand Up @@ -256,7 +256,7 @@ export const usePageIds = (searchKeyword?: string) => {

return useMemo(() => {
if (searchKeyword) {
const filteredPages = produce(pages, (draft) => {
const filteredPages = create(pages, (draft) => {
draft.forEach((page, index) => {
const searchMatches =
page.pageName.toLowerCase().indexOf(searchKeyword.toLowerCase()) >
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/ce/reducers/entityReducers/jsActionsReducer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import {
ReduxActionErrorTypes,
} from "ee/constants/ReduxActionConstants";
import { set, keyBy, findIndex, unset } from "lodash";
import produce from "immer";
import { create } from "mutative";
import { klona } from "klona";

export const initialState: JSCollectionDataState = [];
Expand Down Expand Up @@ -400,7 +400,7 @@ export const handlers = {
}>
>,
) => {
return produce(state, (draft) => {
return create(state, (draft) => {
const CollectionUpdateSearch = keyBy(action.payload, "collectionId");
const actionUpdateSearch = keyBy(action.payload, "id");

Expand Down
4 changes: 2 additions & 2 deletions app/client/src/ce/reducers/uiReducers/applicationsReducer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
defaultNavigationSetting,
defaultThemeSetting,
} from "constants/AppConstants";
import produce from "immer";
import { create } from "mutative";
import { isEmpty } from "lodash";
import type { ApplicationPayload } from "entities/Application";
import { gitConnectSuccess, type GitConnectSuccessPayload } from "git";
Expand Down Expand Up @@ -530,7 +530,7 @@ export const handlers = {
state: ApplicationsReduxState,
action: ReduxAction<NavigationSetting["logoAssetId"]>,
) => {
return produce(state, (draftState: ApplicationsReduxState) => {
return create(state, (draftState: ApplicationsReduxState) => {
draftState.isUploadingNavigationLogo = false;

if (
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/components/editorComponents/WidgetQueryGeneratorForm/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import React, { useCallback, useEffect, useMemo, useState } from "react";
import produce from "immer";
import { create } from "mutative";
import { noop, set } from "lodash";

import { CommonControls } from "./CommonControls";
Expand Down Expand Up @@ -179,7 +179,7 @@ function WidgetQueryGeneratorForm(props: Props) {
setPristine(false);

setConfig(
produce(config, (draftConfig) => {
create(config, (draftConfig) => {
if (
property === "datasource" ||
(typeof property === "object" &&
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/components/propertyControls/ButtonTabControl.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import type { ControlData, ControlProps } from "./BaseControl";
import BaseControl from "./BaseControl";
import type { ToggleGroupOption } from "@appsmith/ads";
import { ToggleButtonGroup } from "@appsmith/ads";
import produce from "immer";
import { create } from "mutative";
import type { DSEventDetail } from "utils/AppsmithUtils";
import {
DSEventTypes,
Expand Down Expand Up @@ -62,7 +62,7 @@ class ButtonTabControl extends BaseControl<ButtonTabControlProps> {
isUpdatedViaKeyboard,
);
} else {
const updatedValues: string[] = produce(values, (draft: string[]) => {
const updatedValues: string[] = create(values, (draft: string[]) => {
draft.push(value);
});
Comment on lines +65 to 67
Copy link
Contributor

@coderabbitai coderabbitai bot Feb 4, 2025

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Simplify array update logic.

Using mutative's create for a simple array push operation is unnecessary overhead. A simpler spread operator would be more efficient here.

-const updatedValues: string[] = create(values, (draft: string[]) => {
-  draft.push(value);
-});
+const updatedValues: string[] = [...values, value];
📝 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 updatedValues: string[] = create(values, (draft: string[]) => {
draft.push(value);
});
const updatedValues: string[] = [...values, value];


Expand Down
5 changes: 1 addition & 4 deletions app/client/src/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,15 +23,12 @@ import "./assets/styles/index.css";
import "./polyfills";
import GlobalStyles from "globalStyles";
// enable autofreeze only in development
import { setAutoFreeze } from "immer";

import AppErrorBoundary from "./AppErrorBoundry";
import log from "loglevel";
import { FaroErrorBoundary } from "@grafana/faro-react";
import { isTracingEnabled } from "instrumentation/utils";

const shouldAutoFreeze = process.env.NODE_ENV === "development";

setAutoFreeze(shouldAutoFreeze);
runSagaMiddleware();

appInitializer();
Expand Down
22 changes: 11 additions & 11 deletions app/client/src/reducers/entityReducers/datasourceReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import type {
import { ToastMessageType } from "entities/Datasource";
import { TEMP_DATASOURCE_ID } from "constants/Datasource";
import type { DropdownOption } from "@appsmith/ads-old";
import produce from "immer";
import { create } from "mutative";
import { assign } from "lodash";

export interface DatasourceDataState {
Expand Down Expand Up @@ -466,7 +466,7 @@ const datasourceReducer = createReducer(initialState, {
state: DatasourceDataState,
action: ReduxAction<Datasource>,
): DatasourceDataState => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.loading = false;
draftState.list.forEach((datasource) => {
if (datasource.id === action.payload.id) {
Expand Down Expand Up @@ -656,15 +656,15 @@ const datasourceReducer = createReducer(initialState, {
[ReduxActionTypes.FETCH_GSHEET_SPREADSHEETS]: (
state: DatasourceDataState,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.isFetchingSpreadsheets = true;
});
},
[ReduxActionTypes.FETCH_GSHEET_SPREADSHEETS_SUCCESS]: (
state: DatasourceDataState,
action: ReduxAction<{ id: string; data: DropdownOption[] }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.spreadsheets[action.payload.id] = {
value: action.payload.data,
};
Expand All @@ -676,7 +676,7 @@ const datasourceReducer = createReducer(initialState, {
state: DatasourceDataState,
action: ReduxAction<{ id: string; error: string }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.spreadsheets[action.payload.id] = {
error: action.payload.error,
};
Expand All @@ -685,15 +685,15 @@ const datasourceReducer = createReducer(initialState, {
});
},
[ReduxActionTypes.FETCH_GSHEET_SHEETS]: (state: DatasourceDataState) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.isFetchingSheets = true;
});
},
[ReduxActionTypes.FETCH_GSHEET_SHEETS_SUCCESS]: (
state: DatasourceDataState,
action: ReduxAction<{ id: string; data: DropdownOption[] }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.sheets[action.payload.id] = {
value: action.payload.data,
};
Expand All @@ -704,23 +704,23 @@ const datasourceReducer = createReducer(initialState, {
state: DatasourceDataState,
action: ReduxAction<{ id: string; error: string }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.sheets[action.payload.id] = {
error: action.payload.error,
};
draftState.gsheetStructure.isFetchingSheets = false;
});
},
[ReduxActionTypes.FETCH_GSHEET_COLUMNS]: (state: DatasourceDataState) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.isFetchingColumns = true;
});
},
[ReduxActionTypes.FETCH_GSHEET_COLUMNS_SUCCESS]: (
state: DatasourceDataState,
action: ReduxAction<{ id: string; data: DropdownOption[] }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.columns[action.payload.id] = {
value: action.payload.data,
};
Expand All @@ -731,7 +731,7 @@ const datasourceReducer = createReducer(initialState, {
state: DatasourceDataState,
action: ReduxAction<{ id: string; error: string }>,
) => {
return produce(state, (draftState) => {
return create(state, (draftState) => {
draftState.gsheetStructure.columns[action.payload.id] = {
error: action.payload.error,
};
Expand Down
8 changes: 4 additions & 4 deletions app/client/src/reducers/entityReducers/metaReducer/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import {
ReduxActionTypes,
WidgetReduxActionTypes,
} from "ee/constants/ReduxActionConstants";
import produce from "immer";
import { create } from "mutative";
import type { EvalMetaUpdates } from "ee/workers/common/DataTreeEvaluator/types";
import {
getMetaWidgetResetObj,
Expand All @@ -38,7 +38,7 @@ export const metaReducer = createReducer(initialState, {
state: MetaState,
action: ReduxAction<UpdateWidgetMetaPropertyPayload>,
) => {
const nextState = produce(state, (draftMetaState) => {
const nextState = create(state, (draftMetaState) => {
set(
draftMetaState,
`${action.payload.widgetId}.${action.payload.propertyName}`,
Expand All @@ -54,7 +54,7 @@ export const metaReducer = createReducer(initialState, {
state: MetaState,
action: ReduxAction<BatchUpdateWidgetMetaPropertyPayload>,
) => {
const nextState = produce(state, (draftMetaState) => {
const nextState = create(state, (draftMetaState) => {
const { batchMetaUpdates } = action.payload;

batchMetaUpdates.forEach(({ propertyName, propertyValue, widgetId }) => {
Expand All @@ -70,7 +70,7 @@ export const metaReducer = createReducer(initialState, {
state: MetaState,
action: ReduxAction<UpdateWidgetMetaPropertyPayload>,
) => {
const nextState = produce(state, (draftMetaState) => {
const nextState = create(state, (draftMetaState) => {
set(
draftMetaState,
`${action.payload.widgetId}.${action.payload.propertyName}`,
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/reducers/entityReducers/metaReducer/metaReducerUtils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import type {
import type { MetaState, WidgetMetaState } from ".";
import type { ReduxAction } from "actions/ReduxActionTypes";
import type { EvalMetaUpdates } from "ee/workers/common/DataTreeEvaluator/types";
import produce from "immer";
import { create } from "mutative";
import { set, unset } from "lodash";
import { klonaRegularWithTelemetry } from "utils/helpers";

Expand Down Expand Up @@ -82,7 +82,7 @@ export function getNextMetaStateWithUpdates(
if (!evalMetaUpdates.length) return state;

// if metaObject is updated in dataTree we also update meta values, to keep meta state in sync.
const newMetaState = produce(state, (draftMetaState) => {
const newMetaState = create(state, (draftMetaState) => {
evalMetaUpdates.forEach(({ metaPropertyPath, value, widgetId }) => {
set(draftMetaState, [widgetId, ...metaPropertyPath], value);
});
Expand Down
17 changes: 6 additions & 11 deletions app/client/src/reducers/evaluationReducers/treeReducer.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,9 @@
import type { ReduxAction } from "actions/ReduxActionTypes";
import { ReduxActionTypes } from "ee/constants/ReduxActionConstants";
import { applyChange } from "deep-diff";
import { applyChange, type Diff } from "deep-diff";
import type { DataTree } from "entities/DataTree/dataTreeTypes";
import { createImmerReducer } from "utils/ReducerUtils";
import * as Sentry from "@sentry/react";
import type { DiffWithNewTreeState } from "workers/Evaluation/helpers";

export type EvaluatedTreeState = DataTree;

Expand All @@ -15,7 +14,7 @@ const evaluatedTreeReducer = createImmerReducer(initialState, {
state: EvaluatedTreeState,
action: ReduxAction<{
dataTree: DataTree;
updates: DiffWithNewTreeState[];
updates: Diff<DataTree, DataTree>[];
removedPaths: [string];
}>,
) => {
Expand All @@ -27,15 +26,11 @@ const evaluatedTreeReducer = createImmerReducer(initialState, {

for (const update of updates) {
try {
if (update.kind === "newTree") {
return update.rhs;
} else {
if (!update.path || update.path.length === 0) {
continue;
}

applyChange(state, undefined, update);
if (!update.path || update.path.length === 0) {
continue;
}

applyChange(state, undefined, update);
} catch (e) {
Sentry.captureException(e, {
extra: {
Expand Down
4 changes: 2 additions & 2 deletions app/client/src/sagas/WidgetAdditionSagas.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
} from "constants/WidgetConstants";
import { toast } from "@appsmith/ads";
import type { DataTree } from "entities/DataTree/dataTreeTypes";
import produce from "immer";
import { create } from "mutative";
import { klona as clone } from "klona/full";
import { getWidgetMinMaxDimensionsInPixel } from "layoutSystems/autolayout/utils/flexWidgetUtils";
import { ResponsiveBehavior } from "layoutSystems/common/utils/constants";
Expand Down Expand Up @@ -112,7 +112,7 @@ function* getChildWidgetProps(
// if (props) props.children = [];

if (props) {
props = produce(props, (draft: WidgetProps) => {
props = create(props, (draft) => {
if (!draft.children || !Array.isArray(draft.children)) {
draft.children = [];
}
Expand Down
Loading
Loading