Skip to content

Release4 #36600

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
vsvamsi1 wants to merge 4 commits into from
Closed

Release4 #36600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ed568ef
webpack config
vsvamsi1 Sep 28, 2024
851a757
fixed one click binding
vsvamsi1 Sep 29, 2024
01b7623
Revert "webpack config"
vsvamsi1 Sep 29, 2024
5f18be3
small fix
vsvamsi1 Oct 1, 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
2 changes: 1 addition & 1 deletion app/client/src/WidgetQueryGenerators/GSheets/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -259,7 +259,7 @@ export default abstract class GSheets extends BaseQueryGenerator {
};
}

static build(
static async build(
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

💡 Codebase verification

Recommendation on async Keyword in GSheets.build Method

It appears that the build method in GSheets does not perform any asynchronous operations. Therefore, the async keyword may be unnecessary. I recommend removing the async keyword to simplify the code unless there are plans to introduce asynchronous functionality in the near future.

  • File: app/client/src/WidgetQueryGenerators/GSheets/index.ts
  • Method: static build
🔗 Analysis chain

Class, let's examine this change closely.

The addition of the async keyword to the build method is an interesting modification. While it doesn't appear to introduce any immediate issues, I have a few points for us to consider:

  1. This change suggests that we're preparing for asynchronous operations within this method. However, I don't see any await keywords or Promise-returning function calls in the method body.

  2. If there are plans to introduce asynchronous operations in the future, it would be beneficial to add a comment explaining this intention. This will help your fellow students... I mean, developers, understand the reasoning behind this change.

  3. If there are no immediate plans for asynchronous operations, we should consider whether this change is necessary at this time.

To ensure we fully understand the impact of this change, let's do a little homework. Run the following command to check for any other occurrences of this method in the codebase:

This will help us identify any areas where we might need to update method calls or handle the returned Promise.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for occurrences of the GSheets.build method in the codebase

# Test: Search for GSheets.build method calls
rg --type typescript "GSheets\.build\(" -A 5

Length of output: 81

Script:

#!/bin/bash
# Description: Check for occurrences of the GSheets.build method in the codebase

# Test: Search for GSheets.build method calls in TypeScript files
rg --type ts "GSheets\.build\(" -A 5

Length of output: 3143

Script:

#!/bin/bash
# Description: Check for asynchronous operations within the GSheets.build method

# Search for 'await' or Promise-related patterns inside the build method
ast-grep --lang typescript --pattern $'
class GSheets {
  $$$
  static async build($_, $_) {
    $$$
  }
  $$$
}' index.ts | rg "await|Promise\.|\.then\("

Length of output: 211

Script:

#!/bin/bash
# Description: Locate all instances of GSheets.build method to identify the correct file path

# Search for the definition of GSheets.build across the codebase
rg "class\s+GSheets" --type ts --context 5

Length of output: 904

Script:

#!/bin/bash
# Description: Check for asynchronous operations within the GSheets.build method

# Search for 'await' or Promise-related patterns inside the build method in the correct file path
ast-grep --lang typescript --pattern $'
class GSheets {
  $$$
  static async build($_, $_) {
    $$$
  }
  $$$
}' app/client/src/WidgetQueryGenerators/GSheets/index.ts | rg "await|Promise\.|\.then\("

Length of output: 200

widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
pluginInitalValues: { actionConfiguration: ActionConfigurationGSheets },
Expand Down
9 changes: 5 additions & 4 deletions app/client/src/WidgetQueryGenerators/MSSQL/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { BaseQueryGenerator } from "../BaseQueryGenerator";
import { formatDialect, sql } from "sql-formatter";
import type {
ActionConfigurationSQL,
WidgetQueryGenerationConfig,
Expand All @@ -11,7 +10,7 @@ import without from "lodash/without";
import { DatasourceConnectionMode } from "entities/Datasource";

export default abstract class MSSQL extends BaseQueryGenerator {
private static buildSelect(
private static async buildSelect(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
) {
Expand Down Expand Up @@ -77,6 +76,8 @@ export default abstract class MSSQL extends BaseQueryGenerator {
{ template: "", params: [] } as { template: string; params: string[] },
);

const { formatDialect, sql } = await import("sql-formatter");

Comment on lines +79 to +80
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider the impact of dynamic imports on performance.

Dynamically importing sql-formatter within the buildSelect method can introduce performance overhead, especially if this method is called frequently. If sql-formatter is essential for most operations, it's better to statically import it at the top of the file. This approach reduces the runtime cost of importing modules during function execution.

Apply the following changes:

At the top of the file, add:

+import { formatDialect, sql } from "sql-formatter";

Then, remove the dynamic import inside the buildSelect method:

-const { formatDialect, sql } = await import("sql-formatter");

This adjustment will improve the efficiency of your code by loading the module once during the initial load rather than on every function call.

📝 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 { formatDialect, sql } = await import("sql-formatter");
import { formatDialect, sql } from "sql-formatter";
// The line below should be removed
// const { formatDialect, sql } = await import("sql-formatter");

//formats sql string
const res = formatDialect(template, {
params,
Expand Down Expand Up @@ -196,15 +197,15 @@ export default abstract class MSSQL extends BaseQueryGenerator {
};
}

public static build(
public static async build(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
pluginInitalValues: { actionConfiguration: ActionConfigurationSQL },
) {
const allBuildConfigs = [];

if (widgetConfig.select) {
allBuildConfigs.push(this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace 'this' with the class name for clarity in static methods.

In static contexts, using this can be confusing because it refers to the class itself, not an instance. To enhance readability and maintainability, it's better to use the class name when calling other static methods within the class.

Apply the following diff to make the change:

-allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
+allBuildConfigs.push(await MSSQL.buildSelect(widgetConfig, formConfig));

By explicitly using MSSQL, you make it clear that you're referencing the class, which helps other developers understand the code more easily.

📝 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
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await MSSQL.buildSelect(widgetConfig, formConfig));
🧰 Tools
🪛 Biome

[error] 208-208: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

}

if (
Expand Down
8 changes: 4 additions & 4 deletions app/client/src/WidgetQueryGenerators/MySQL/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { BaseQueryGenerator } from "../BaseQueryGenerator";
import { formatDialect, mysql } from "sql-formatter";
import type {
ActionConfigurationSQL,
WidgetQueryGenerationConfig,
Expand All @@ -11,7 +10,7 @@ import without from "lodash/without";
import { DatasourceConnectionMode } from "entities/Datasource";

export default abstract class MySQL extends BaseQueryGenerator {
private static buildSelect(
private static async buildSelect(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
) {
Expand Down Expand Up @@ -76,6 +75,7 @@ export default abstract class MySQL extends BaseQueryGenerator {
},
{ template: "", params: [] } as { template: string; params: string[] },
);
const { formatDialect, mysql } = await import("sql-formatter");

//formats sql string
const res = formatDialect(template, {
Expand Down Expand Up @@ -196,15 +196,15 @@ export default abstract class MySQL extends BaseQueryGenerator {
};
}

public static build(
public static async build(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
pluginInitalValues: { actionConfiguration: ActionConfigurationSQL },
) {
const allBuildConfigs = [];

if (widgetConfig.select) {
allBuildConfigs.push(this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Clarify Static Context by Replacing this with Class Name

Using this within a static method can be misleading because it refers to the class itself, not an instance. To enhance readability and avoid confusion, it's better to use the class name directly.

Consider updating the code as follows:

-allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
+allBuildConfigs.push(await MySQL.buildSelect(widgetConfig, formConfig));
📝 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
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await MySQL.buildSelect(widgetConfig, formConfig));
🧰 Tools
🪛 Biome

[error] 207-207: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

}

if (
Expand Down
9 changes: 5 additions & 4 deletions app/client/src/WidgetQueryGenerators/PostgreSQL/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { BaseQueryGenerator } from "../BaseQueryGenerator";
import { formatDialect, postgresql } from "sql-formatter";
import type {
ActionConfigurationSQL,
WidgetQueryGenerationConfig,
Expand All @@ -11,7 +10,7 @@ import { without } from "lodash";
import { DatasourceConnectionMode } from "entities/Datasource";

export default abstract class PostgreSQL extends BaseQueryGenerator {
private static buildSelect(
private static async buildSelect(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
) {
Expand Down Expand Up @@ -87,6 +86,8 @@ export default abstract class PostgreSQL extends BaseQueryGenerator {
{ template: "", params: {} },
);
//formats sql string
const { formatDialect, postgresql } = await import("sql-formatter");

const res = formatDialect(template, {
Comment on lines +89 to 91
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider statically importing 'sql-formatter' for improved performance

While dynamic imports are useful, importing sql-formatter statically at the top of your file can reduce the overhead associated with dynamic imports, especially if buildSelect is called frequently.

You can move the import to the top of your file:

import { formatDialect, postgresql } from "sql-formatter";

And then remove the dynamic import inside buildSelect:

- const { formatDialect, postgresql } = await import("sql-formatter");

params,
dialect: postgresql,
Expand Down Expand Up @@ -205,15 +206,15 @@ export default abstract class PostgreSQL extends BaseQueryGenerator {
};
}

public static build(
public static async build(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
pluginInitalValues: { actionConfiguration: ActionConfigurationSQL },
) {
const allBuildConfigs = [];

if (widgetConfig.select) {
allBuildConfigs.push(this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 29, 2024

Choose a reason for hiding this comment

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

⚠️ Potential issue

Replace 'this' with the class name in static context

In static methods, using this can be confusing because it refers to the class itself. To improve clarity, it's better to use the class name directly when calling other static methods.

Apply this diff to correct the issue:

- allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
+ allBuildConfigs.push(await PostgreSQL.buildSelect(widgetConfig, formConfig));

Committable suggestion was skipped due to low confidence.

🧰 Tools
🪛 Biome

[error] 217-217: Using this in a static context can be confusing.

this refers to the class.
Unsafe fix: Use the class name instead.

(lint/complexity/noThisInStatic)

}

if (
Expand Down
9 changes: 5 additions & 4 deletions app/client/src/WidgetQueryGenerators/Snowflake/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { BaseQueryGenerator } from "../BaseQueryGenerator";
import { formatDialect, snowflake } from "sql-formatter";
import { QUERY_TYPE } from "../types";
import type {
WidgetQueryGenerationConfig,
Expand All @@ -11,7 +10,7 @@ import { without } from "lodash";
import { DatasourceConnectionMode } from "entities/Datasource";

export default abstract class Snowflake extends BaseQueryGenerator {
private static buildSelect(
private static async buildSelect(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
) {
Expand Down Expand Up @@ -78,6 +77,8 @@ export default abstract class Snowflake extends BaseQueryGenerator {
);

//formats sql string
const { formatDialect, snowflake } = await import("sql-formatter");

const res = formatDialect(template, {
params,
dialect: snowflake,
Expand Down Expand Up @@ -199,15 +200,15 @@ export default abstract class Snowflake extends BaseQueryGenerator {
};
}

public static build(
public static async build(
widgetConfig: WidgetQueryGenerationConfig,
formConfig: WidgetQueryGenerationFormConfig,
pluginInitalValues: { actionConfiguration: ActionConfigurationSQL },
) {
const allBuildConfigs = [];

if (widgetConfig.select) {
allBuildConfigs.push(this.buildSelect(widgetConfig, formConfig));
allBuildConfigs.push(await this.buildSelect(widgetConfig, formConfig));
}

if (
Expand Down
3 changes: 2 additions & 1 deletion app/client/src/sagas/OneClickBindingSaga.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,7 +136,8 @@ function* BindWidgetToDatasource(
plugin.packageName,
);

const actionConfigurationList = widgetQueryGenerator.build(
const actionConfigurationList = yield call(
widgetQueryGenerator.build,
widgetQueryGenerationConfig,
action.payload,
defaultValues,
Expand Down