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

chore: decouple widget-config.json from main chunk #36924

Merged
merged 7 commits into from
Oct 17, 2024
Merged

chore: decouple widget-config.json from main chunk #36924

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3e86da9
decouple widget-config.json from main chunk
vsvamsi1 Oct 17, 2024
5aeea62
type fix
vsvamsi1 Oct 17, 2024
6501304
small fix
vsvamsi1 Oct 17, 2024
62284ec
review comments
vsvamsi1 Oct 17, 2024
32806b9
review comment
vsvamsi1 Oct 17, 2024
7307a7d
code rabbit fix
vsvamsi1 Oct 17, 2024
ae14b3f
small fix
vsvamsi1 Oct 17, 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
12 changes: 6 additions & 6 deletions app/client/packages/dsl/src/migrate/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ const migrateUnversionedDSL = (currentDSL: DSLWidget) => {
// A rudimentary transform function which updates the DSL based on its version.
// A more modular approach needs to be designed.
// This needs the widget config to be already built to migrate correctly
const migrateVersionedDSL = (currentDSL: DSLWidget, newPage = false) => {
const migrateVersionedDSL = async (currentDSL: DSLWidget, newPage = false) => {
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

Ensure proper error handling in 'migrateVersionedDSL'

When converting migrateVersionedDSL into an asynchronous function, it's important to handle any potential errors that may occur during asynchronous operations. Remember, unhandled promise rejections can lead to unexpected behaviors in your application. Consider adding error handling, such as try/catch blocks, to make the function more robust.

if (currentDSL.version === 1) {
if (currentDSL.children && currentDSL.children.length > 0)
currentDSL.children = currentDSL.children.map(updateContainers);
Expand Down Expand Up @@ -205,7 +205,7 @@ const migrateVersionedDSL = (currentDSL: DSLWidget, newPage = false) => {
}

if (currentDSL.version === 12) {
currentDSL = migrateIncorrectDynamicBindingPathLists(currentDSL);
currentDSL = await migrateIncorrectDynamicBindingPathLists(currentDSL);
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

Properly handle errors when awaiting 'migrateIncorrectDynamicBindingPathLists'

Awaiting the asynchronous function migrateIncorrectDynamicBindingPathLists means that any errors it may throw will propagate up the call stack. To ensure reliability, please implement appropriate error handling within migrateVersionedDSL, potentially using a try/catch block around this await call.

currentDSL.version = 13;
}

Expand Down Expand Up @@ -619,15 +619,15 @@ const migrateVersionedDSL = (currentDSL: DSLWidget, newPage = false) => {
return currentDSL;
};

export const migrateDSL = (
export const migrateDSL = async (
currentDSL: DSLWidget,
newPage = false,
): DSLWidget => {
): Promise<DSLWidget> => {
Comment on lines +622 to +625
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

Ensure error handling in the asynchronous function 'migrateDSL'

Now that migrateDSL is an asynchronous function, it's crucial to handle any errors that might occur during its execution. Incorporating error-handling mechanisms will enhance the robustness of your migration process.

if (currentDSL.version === undefined) {
const initialDSL = migrateUnversionedDSL(currentDSL);

return migrateVersionedDSL(initialDSL, newPage) as DSLWidget;
return (await migrateVersionedDSL(initialDSL, newPage)) as DSLWidget;
} else {
return migrateVersionedDSL(currentDSL, newPage) as DSLWidget;
return (await migrateVersionedDSL(currentDSL, newPage)) as DSLWidget;
}
};
62 changes: 49 additions & 13 deletions ...t/packages/dsl/src/migrate/migrations/012-migrate-incorrect-dynamic-binding-path-lists.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import isString from "lodash/isString";
import memoize from "micro-memoize";
import { isObject, isUndefined } from "lodash";
import { generateReactKey, isDynamicValue } from "../utils";
import widgetConfigs from "../helpers/widget-configs.json";

export const WidgetHeightLimits = {
MAX_HEIGHT_IN_ROWS: 9000,
Expand Down Expand Up @@ -511,8 +510,33 @@ export function addSearchConfigToPanelConfig(config: readonly any[]) {
return configItem;
});
}
// Cache for lazy-loaded widget configurations
let cachedWidgetConfigs: any | null = null;

/*
Lazily load this file since it is very large and used in migrations for certain DSL versions. By lazily loading
this large file it can be reduce the main chunk only be loaded for certain limited conditions.
*/
Comment on lines +516 to +519
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

Improve the clarity of the comment block

The multi-line comment contains grammatical errors that could affect comprehension. Let's revise it for better readability:

/*
- Lazily load this file since it is very large and used in migrations for certain DSL versions. By lazily loading 
- this large file it can be reduce the main chunk only be loaded for certain limited conditions.
+ Lazily load this file since it is very large and used in migrations for certain DSL versions. By lazily loading 
+ this large file, we can reduce the main chunk size, ensuring it is only loaded under certain limited conditions.
*/
📝 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
/*
Lazily load this file since it is very large and used in migrations for certain DSL versions. By lazily loading
this large file it can be reduce the main chunk only be loaded for certain limited conditions.
*/
/*
Lazily load this file since it is very large and used in migrations for certain DSL versions. By lazily loading
this large file, we can reduce the main chunk size, ensuring it is only loaded under certain limited conditions.
*/

const loadWidgetConfig = async () => {
if (!cachedWidgetConfigs) {
try {
const { default: widgetConfigs } = await import(
"../helpers/widget-configs.json"
);

cachedWidgetConfigs = widgetConfigs; // Cache the module for future use
} catch (e) {
log.error("Error loading WidgetConfig", e);
}
}

const getWidgetPropertyPaneContentConfig = (type: string): readonly any[] => {
return cachedWidgetConfigs;
};

const getWidgetPropertyPaneContentConfig = async (
type: string,
): Promise<readonly any[]> => {
const widgetConfigs = await loadWidgetConfig();
const propertyPaneContentConfig = (widgetConfigs as any)[type]
.propertyPaneContentConfig;

Expand Down Expand Up @@ -540,7 +564,11 @@ const getWidgetPropertyPaneContentConfig = (type: string): readonly any[] => {
}
};

const getWidgetPropertyPaneStyleConfig = (type: string): readonly any[] => {
const getWidgetPropertyPaneStyleConfig = async (
type: string,
): Promise<readonly any[]> => {
const widgetConfigs = await loadWidgetConfig();

const propertyPaneStyleConfig = (widgetConfigs as any)[type]
.propertyPaneStyleConfig;

Comment on lines +570 to 574
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

Add null check for widgetConfigs in getWidgetPropertyPaneStyleConfig

Just as before, we should verify that widgetConfigs is not null to prevent potential runtime errors. Consistent error handling improves code reliability.

Consider adding a null check:

const widgetConfigs = await loadWidgetConfig();

+ if (!widgetConfigs) {
+   throw new Error("Failed to load widget configurations");
+ }

const propertyPaneStyleConfig = (widgetConfigs as any)[type]
  .propertyPaneStyleConfig;
📝 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 widgetConfigs = await loadWidgetConfig();
const propertyPaneStyleConfig = (widgetConfigs as any)[type]
.propertyPaneStyleConfig;
const widgetConfigs = await loadWidgetConfig();
if (!widgetConfigs) {
throw new Error("Failed to load widget configurations");
}
const propertyPaneStyleConfig = (widgetConfigs as any)[type]
.propertyPaneStyleConfig;

Expand All @@ -567,14 +595,20 @@ const getWidgetPropertyPaneStyleConfig = (type: string): readonly any[] => {
}
};

const getWidgetPropertyPaneCombinedConfig = (type: string): readonly any[] => {
const contentConfig = getWidgetPropertyPaneContentConfig(type);
const styleConfig = getWidgetPropertyPaneStyleConfig(type);
const getWidgetPropertyPaneCombinedConfig = async (
type: string,
): Promise<readonly any[]> => {
const contentConfig = await getWidgetPropertyPaneContentConfig(type);
const styleConfig = await getWidgetPropertyPaneStyleConfig(type);

return [...contentConfig, ...styleConfig];
};

const getWidgetPropertyPaneConfig = (type: string): readonly any[] => {
const getWidgetPropertyPaneConfig = async (
type: string,
): Promise<readonly any[]> => {
const widgetConfigs = await loadWidgetConfig();

const propertyPaneConfig = (widgetConfigs as any)[type].propertyPaneConfig;

const features = (widgetConfigs as any)[type].features;
Comment on lines +610 to 614
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

Add null check for widgetConfigs in getWidgetPropertyPaneConfig

Again, let's ensure that widgetConfigs is not null before accessing its properties. This practice helps prevent unexpected crashes.

Here's the suggested modification:

const widgetConfigs = await loadWidgetConfig();

+ if (!widgetConfigs) {
+   throw new Error("Failed to load widget configurations");
+ }

const propertyPaneConfig = (widgetConfigs as any)[type].propertyPaneConfig;

Committable suggestion was skipped due to low confidence.

Expand All @@ -590,7 +624,7 @@ const getWidgetPropertyPaneConfig = (type: string): readonly any[] => {

return enhancedPropertyPaneConfig;
} else {
const config = getWidgetPropertyPaneCombinedConfig(type);
const config = await getWidgetPropertyPaneCombinedConfig(type);

if (config === undefined) {
log.error("Widget property pane config not defined", type);
Expand Down Expand Up @@ -957,14 +991,14 @@ const getAllPathsFromPropertyConfig = memoize(
{ maxSize: 1000 },
);

export const migrateIncorrectDynamicBindingPathLists = (
export const migrateIncorrectDynamicBindingPathLists = async (
currentDSL: Readonly<DSLWidget>,
): DSLWidget => {
): Promise<DSLWidget> => {
const migratedDsl = {
...currentDSL,
};
const dynamicBindingPathList: any[] = [];
const propertyPaneConfig = getWidgetPropertyPaneConfig(currentDSL.type);
const propertyPaneConfig = await getWidgetPropertyPaneConfig(currentDSL.type);
const { bindingPaths } = getAllPathsFromPropertyConfig(
currentDSL,
propertyPaneConfig,
Expand All @@ -984,8 +1018,10 @@ export const migrateIncorrectDynamicBindingPathLists = (
migratedDsl.dynamicBindingPathList = dynamicBindingPathList;

if (currentDSL.children) {
migratedDsl.children = currentDSL.children.map(
migrateIncorrectDynamicBindingPathLists,
migratedDsl.children = await Promise.all(
currentDSL.children.map(async (value) =>
migrateIncorrectDynamicBindingPathLists(value),
Comment on lines +1022 to +1023
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

Simplify the map function inside Promise.all

Recall that if an arrow function doesn't use await, we don't need the async keyword. Removing it simplifies the code without changing its functionality.

Here's the simplified code:

-      currentDSL.children.map(async (value) =>
+      currentDSL.children.map((value) =>
        migrateIncorrectDynamicBindingPathLists(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
currentDSL.children.map(async (value) =>
migrateIncorrectDynamicBindingPathLists(value),
currentDSL.children.map((value) =>
migrateIncorrectDynamicBindingPathLists(value),

),
);
}

Expand Down
127 changes: 62 additions & 65 deletions app/client/packages/dsl/src/migrate/tests/DSLMigration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -938,85 +938,82 @@ describe("Test all the migrations are running", () => {
afterAll(() => {
jest.clearAllMocks();
});
migrations.forEach((migration: Migration) => {
/**
* Generates mock fucntion for each migration function.
* Mocks the implementation
*/
const version = migration.version ?? 0;
test("assert migration functions being called when migrate dsl has been called ", async () => {
migrations.forEach((migration: Migration) => {
/**
* Generates mock fucntion for each migration function.
* Mocks the implementation
*/
const version = migration.version ?? 0;

mockFnObj[version] = [];
mockFnObj[version] = [];

migration.functionLookup.forEach((lookup) => {
const { functionName, moduleObj } = lookup;
migration.functionLookup.forEach((lookup) => {
const { functionName, moduleObj } = lookup;

if (moduleObj) {
mockFnObj[version].push({
spyOnFunc: jest
.spyOn(moduleObj, functionName)
.mockImplementation((dsl: any) => {
/**
* We need to delete the children property on the first migration(calculateDynamicHeight),
* to avoid the recursion in the second migration(updateContainers)
*/
dsl && delete dsl.children;
if (moduleObj) {
mockFnObj[version].push({
spyOnFunc: jest
.spyOn(moduleObj, functionName)
.mockImplementation((dsl: any) => {
/**
* We need to delete the children property on the first migration(calculateDynamicHeight),
* to avoid the recursion in the second migration(updateContainers)
*/
dsl && delete dsl.children;

return {
version: dsl?.version,
validationFuncName: functionName,
};
}),
});
}
return {
version: dsl?.version,
validationFuncName: functionName,
};
}),
});
}
});
});
});

// Runs all the migrations
DSLMigrations.migrateDSL(originalDSLForDSLMigrations as unknown as DSLWidget);
// Runs all the migrations
await DSLMigrations.migrateDSL(
originalDSLForDSLMigrations as unknown as DSLWidget,
);

migrations.forEach((item: any, testIdx: number) => {
const { functionLookup, version } = item;
const dslVersion = version ?? 0;
migrations.forEach((item: any) => {
const { functionLookup, version } = item;
const dslVersion = version ?? 0;

functionLookup.forEach(
(lookup: { moduleObj: any; functionName: string }, index: number) => {
const { functionName, moduleObj } = lookup;
functionLookup.forEach(
(lookup: { moduleObj: any; functionName: string }, index: number) => {
const { functionName, moduleObj } = lookup;

if (moduleObj) {
const mockObj = mockFnObj[dslVersion][index].spyOnFunc;
const calls = mockObj.mock?.calls;
const results = mockObj.mock?.results;
const resultsLastIdx = mockObj.mock.results.length - 1;

describe(`Test ${testIdx}:`, () => {
test(`Has ${functionName} function executed?`, () => {
// Check if the migration function is called
expect(results[resultsLastIdx].value.validationFuncName).toEqual(
functionName,
);
});
if (moduleObj) {
const mockObj = mockFnObj[dslVersion][index].spyOnFunc;
const calls = mockObj.mock?.calls;
const results = mockObj.mock?.results;
const resultsLastIdx = mockObj.mock.results.length - 1;

// Check if the migration function is called
expect(results[resultsLastIdx].value.validationFuncName).toEqual(
functionName,
);
// Check if the migration function is called with the current DSL version
calls.forEach((args: any) => {
test(`Does ${functionName} executes with DSL version: ${version}?`, () => {
if (args[0]?.version === version) {
expect(args[0]?.version).toEqual(version);
}
});
test(`For ${functionName}, is the ${args[0]?.version} registerd in tests?`, () => {
expect(
Object.keys(mockFnObj).includes(
args[0]?.version.toString() ?? "0",
),
).toBe(true);
});
// Does functionName executes with the correct DSL version number
if (args[0]?.version === version) {
expect(args[0]?.version).toEqual(version);
}

// For a functionName is the correct DSL version registed in test cases
expect(
Object.keys(mockFnObj).includes(
args[0]?.version.toString() ?? "0",
),
).toBe(true);
});
});
}
},
);
}
},
);
});
});

test("Check the migration count matches the lates page version", () => {
expect(migrations.length).toEqual(DSLMigrations.LATEST_DSL_VERSION);
});
Expand Down
12 changes: 6 additions & 6 deletions app/client/packages/dsl/src/migrate/tests/DSLMigrationsUtils.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import type { DSLWidget } from "../types";
const ASSETS_CDN_URL = "https://assets.appsmith.com";

describe("correctly migrate dsl", () => {
it("migrateDSL for private widget", () => {
it("migrateDSL for private widget", async () => {
const currentVersion = 49; // before adding privateWidgets to all List widgets
const nextVersion = LATEST_DSL_VERSION; // It runs Two Migrations, Always Update as migration increases

Expand Down Expand Up @@ -1229,12 +1229,12 @@ describe("correctly migrate dsl", () => {
isLoading: false,
};

const actualNextDsl = migrateDSL(currentDSL);
const actualNextDsl = await migrateDSL(currentDSL);

expect(actualNextDsl).toEqual(expectedNextDSL);
});

it("migrateDSL for theming v1", () => {
it("migrateDSL for theming v1", async () => {
const currentVersion = 53;
const nextVersion = LATEST_DSL_VERSION;
const currentDSL: DSLWidget = {
Expand Down Expand Up @@ -2452,7 +2452,7 @@ describe("correctly migrate dsl", () => {
isLoading: false,
};

const actualNextDsl = migrateDSL(currentDSL);
const actualNextDsl = await migrateDSL(currentDSL);

expect(actualNextDsl).toEqual(expectedNextDSL);
});
Expand Down Expand Up @@ -3096,7 +3096,7 @@ describe("correctly migrate dsl", () => {
expect(actualNextDSL).toEqual(expectedNextDSL);
});

it("correctly migrates currentIndex/currentRow properties for validations in table view", () => {
it("correctly migrates currentIndex/currentRow properties for validations in table view", async () => {
const currentVersion = 78;
const currentDSL: DSLWidget = {
bottomRow: 740,
Expand Down Expand Up @@ -3171,7 +3171,7 @@ describe("correctly migrate dsl", () => {
},
],
};
const nextDSL = migrateDSL(currentDSL);
const nextDSL = await migrateDSL(currentDSL);

// eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
const validations = (nextDSL.children || [])[0].primaryColumns.column1
Expand Down
4 changes: 2 additions & 2 deletions app/client/packages/rts/src/controllers/Dsl/DslController.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ export default class DSLController extends BaseController {
super();
}

migrateDSL(req: Request, res: Response) {
async migrateDSL(req: Request, res: Response) {
try {
const latestDSL = migrateDSLToLatest(req.body);
const latestDSL = await migrateDSLToLatest(req.body);

super.sendResponse(res, latestDSL);
} catch (err) {
Expand Down
8 changes: 7 additions & 1 deletion app/client/packages/rts/src/routes/dsl_routes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import DSLController from "@controllers/Dsl/DslController";
import { Validator } from "@middlewares/Validator";
import express from "express";
import type { Response, Request } from "express";

const router = express.Router();
const dslController = new DSLController();
Expand All @@ -12,6 +13,11 @@ router.get(
dslController.getLatestDSLVersion,
);

router.post("/migrate", validator.validateRequest, dslController.migrateDSL);
router.post(
"/migrate",
validator.validateRequest,
async (req: Request, res: Response) =>
await dslController.migrateDSL(req, res),
);

export default router;
Loading
Loading