feat: custom eslint rule infra (#38345)

## Description
This PR introduces the custom ESLint rule to [handle floating
promises](https://typescript-eslint.io/rules/no-floating-promises/) in
our JS Objects (using async function calls without await inside async
functions). Along with this, some refactors were needed

1) Function to dynamically enable/disable the rules based on context (
using editor type as the context for now, can be updated as per
requirements).
2) Generate a list of async functions from the global data input for
evaluation.
3) Package added for estree types.

Note: This custom rule is only used in our EE codebase for now. We can
enable it in CE in case the requirement arises.

Fixes https://github.com/appsmithorg/appsmith/issues/37255

## Automation

/test sanity js

### 🔍 Cypress test results
<!-- This is an auto-generated comment: Cypress test results  -->
> [!TIP]
> 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
> Workflow run:
<https://github.com/appsmithorg/appsmith/actions/runs/12558146123>
> Commit: 36b66109c7ef70dc1eb97b445661ac69f881775a
> <a
href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=12558146123&attempt=1"
target="_blank">Cypress dashboard</a>.
> Tags: `@tag.Sanity, @tag.JS`
> Spec:
> <hr>Tue, 31 Dec 2024 10:54:23 UTC
<!-- end of auto-generated comment: Cypress test results  -->


## Communication
Should the DevRel and Marketing teams inform users about this change?
- [ ] Yes
- [x] No


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

## Release Notes

- **New Features**
	- Added a custom ESLint rule to enforce proper Promise handling
	- Enhanced linting capabilities with more granular error detection

- **Improvements**
- Updated linting utility functions to support more flexible
configuration
	- Introduced more detailed context-aware linting options

- **Development**
	- Added TypeScript type definitions for ESTree
	- Expanded test coverage for linting utilities

- **Chores**
	- Refactored linting-related utility functions and constants

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Ayush Pahwa 2025-01-02 19:11:55 +07:00 committed by GitHub
parent 51f3d0ebdf
commit 9c816473ff
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
10 changed files with 469 additions and 39 deletions

View File

@ -261,6 +261,7 @@
"@types/deep-diff": "^1.0.0", "@types/deep-diff": "^1.0.0",
"@types/dom-view-transitions": "^1.0.5", "@types/dom-view-transitions": "^1.0.5",
"@types/downloadjs": "^1.4.2", "@types/downloadjs": "^1.4.2",
"@types/estree": "^1.0.6",
"@types/jest": "^29.5.3", "@types/jest": "^29.5.3",
"@types/jshint": "^2.12.0", "@types/jshint": "^2.12.0",
"@types/lodash": "^4.14.120", "@types/lodash": "^4.14.120",

View File

@ -18,7 +18,9 @@ export const editorType: EditorType = {
[BUILDER_BASE_PATH_DEPRECATED]: EditorNames.APPLICATION, [BUILDER_BASE_PATH_DEPRECATED]: EditorNames.APPLICATION,
}; };
export const useEditorType = (path: string) => { // Utility function to get editor type based on path
// to be used in non react functions
export const getEditorType = (path: string) => {
const basePath = matchPath(path, { const basePath = matchPath(path, {
path: [BUILDER_VIEWER_PATH_PREFIX, BUILDER_BASE_PATH_DEPRECATED], path: [BUILDER_VIEWER_PATH_PREFIX, BUILDER_BASE_PATH_DEPRECATED],
}); });
@ -28,6 +30,11 @@ export const useEditorType = (path: string) => {
: editorType[BUILDER_VIEWER_PATH_PREFIX]; : editorType[BUILDER_VIEWER_PATH_PREFIX];
}; };
// custom hook to get editor type based on path
export const useEditorType = (path: string) => {
return getEditorType(path);
};
export function useOutsideClick<T extends HTMLElement>( export function useOutsideClick<T extends HTMLElement>(
ref: RefObject<T>, ref: RefObject<T>,
inputRef: RefObject<T>, inputRef: RefObject<T>,

View File

@ -0,0 +1,15 @@
interface ContextType {
editorType: string;
}
/**
* Function used to conditionally enable/disable custom rules based on context provided
*
* @returns {Record<string, "off"|"error">} Object with settings for the rule
* */
export const getLintRulesBasedOnContext = ({}: ContextType): Record<
string,
"off" | "error"
> => {
return {};
};

View File

@ -0,0 +1 @@
export * from "ce/utils/lintRulesHelpers";

View File

@ -1,7 +1,8 @@
import { ECMA_VERSION } from "@shared/ast"; import { ECMA_VERSION } from "@shared/ast";
import type { LintOptions } from "jshint"; import type { LintOptions } from "jshint";
import isEntityFunction from "./utils/isEntityFunction"; import isEntityFunction from "./utils/isEntityFunction";
import type { Linter } from "eslint-linter-browserify"; import { noFloatingPromisesLintRule } from "./customRules/no-floating-promises";
import { getLintRulesBasedOnContext } from "ee/utils/lintRulesHelpers";
export enum LINTER_TYPE { export enum LINTER_TYPE {
"JSHINT" = "JSHint", "JSHINT" = "JSHint",
@ -9,7 +10,9 @@ export enum LINTER_TYPE {
} }
export const lintOptions = ( export const lintOptions = (
globalData: Record<string, boolean>, globalData: Record<string, boolean | "readonly" | "writable">,
asyncFunctions: string[],
editorType: string,
linterType: LINTER_TYPE = LINTER_TYPE.JSHINT, linterType: LINTER_TYPE = LINTER_TYPE.JSHINT,
) => { ) => {
if (linterType === LINTER_TYPE.JSHINT) { if (linterType === LINTER_TYPE.JSHINT) {
@ -39,25 +42,28 @@ export const lintOptions = (
loopfunc: true, loopfunc: true,
} as LintOptions; } as LintOptions;
} else { } else {
const eslintGlobals: Record<string, "writable" | "readonly"> = { const extraRules = getLintRulesBasedOnContext({ editorType });
setTimeout: "readonly",
clearTimeout: "readonly",
console: "readonly",
};
for (const key in globalData) {
if (globalData.hasOwnProperty(key)) {
eslintGlobals[key] = "readonly";
}
}
return { return {
languageOptions: { languageOptions: {
ecmaVersion: ECMA_VERSION, ecmaVersion: ECMA_VERSION,
globals: eslintGlobals, globals: globalData,
sourceType: "script", sourceType: "script",
}, },
// Need to pass for custom rules
settings: {
globalData,
asyncFunctions,
},
plugins: {
customRules: {
rules: {
"no-floating-promises": noFloatingPromisesLintRule,
},
},
},
rules: { rules: {
...extraRules,
eqeqeq: "off", eqeqeq: "off",
curly: "off", curly: "off",
"no-extend-native": "error", "no-extend-native": "error",
@ -76,7 +82,7 @@ export const lintOptions = (
"no-unused-expressions": "off", "no-unused-expressions": "off",
"no-loop-func": "off", "no-loop-func": "off",
}, },
} as Linter.Config; } as const;
} }
}; };

View File

@ -0,0 +1,138 @@
import type { Rule } from "eslint";
import type * as ESTree from "estree";
export const noFloatingPromisesLintRule: Rule.RuleModule = {
meta: {
type: "problem",
docs: {
description: "Requires handling of Promises (using await or .then())",
category: "Possible Errors",
recommended: false,
},
messages: {
unhandledPromise:
"Unhandled Promise detected. Handle using await or .then()",
},
schema: [], // Rule does not accept configuration options
},
create: function (context: Rule.RuleContext) {
// Access async functions from settings
const asyncFunctions = context.settings?.asyncFunctions || [];
return {
FunctionDeclaration(node: ESTree.FunctionDeclaration) {
// Start traversal from the function body
traverseNode(node.body, null);
},
};
/**
* Recursively traverses the AST node and its children.
* Processes CallExpressions and continues traversing child nodes.
*/
function traverseNode(
node: ESTree.Node | null,
parent: ESTree.Node | null,
) {
if (!node) return;
if (node.type === "CallExpression") {
checkCallExpression(node as ESTree.CallExpression, parent);
}
// Retrieve keys for child nodes and traverse them
const visitorKeys = context.getSourceCode().visitorKeys[node.type] || [];
for (const key of visitorKeys) {
const child = (node as any)[key]; // eslint-disable-line @typescript-eslint/no-explicit-any
if (Array.isArray(child)) {
child.forEach(
(c) => c && typeof c.type === "string" && traverseNode(c, node),
);
} else if (child && typeof child.type === "string") {
traverseNode(child, node);
}
}
}
/**
* Determines if a node is inside an async function by traversing its parent chain.
*/
function isInAsyncFunction(node: ESTree.Node | null): boolean {
while (node) {
if (
(node.type === "FunctionDeclaration" ||
node.type === "FunctionExpression") &&
node.async
) {
return true;
}
// Move to the parent node in the AST
// @ts-expect-error: Types may not always define `parent`
node = node.parent;
}
return false;
}
/**
* Checks if a CallExpression represents an unhandled Promise.
* Reports an error if the promise is not awaited or chained with `.then()`, `.catch()`, or `.finally()`.
*/
function checkCallExpression(
node: ESTree.CallExpression,
parent: ESTree.Node | null,
) {
const callee = node.callee;
let isPotentialAsyncCall = false;
// Identify async calls by matching against the asyncFunctions list
if (callee.type === "MemberExpression") {
const object = callee.object;
const property = callee.property;
if (
property.type === "Identifier" &&
object.type === "Identifier" &&
asyncFunctions.includes(`${object.name}.${property.name}`)
) {
isPotentialAsyncCall = true;
}
}
// Report if the async call is unhandled and not properly awaited
if (isPotentialAsyncCall && isInAsyncFunction(parent)) {
if (
parent &&
parent.type !== "AwaitExpression" &&
parent.type !== "ReturnStatement" &&
!isHandledWithPromiseMethods(parent)
) {
context.report({
node,
messageId: "unhandledPromise",
});
}
}
}
/**
* Determines if a CallExpression is handled with `.then()`, `.catch()`, or `.finally()`.
*/
function isHandledWithPromiseMethods(parent: ESTree.Node): boolean {
if (
parent.type === "MemberExpression" &&
parent.property &&
["then", "catch", "finally"].includes(
(parent.property as ESTree.Identifier).name,
)
) {
return true;
}
return false;
}
},
};

View File

@ -0,0 +1,99 @@
import { ENTITY_TYPE } from "ee/entities/DataTree/types";
import { LINTER_TYPE } from "../constants";
import { generateLintingGlobalData } from "../utils/getLintingErrors";
jest.mock("ee/hooks", () => {
const actualModule = jest.requireActual("ee/hooks");
return {
...actualModule, // Spread the original module exports
getEditorType: jest.fn(() => "TestEditorType"), // Override `getEditorType`
};
});
describe("generateLintingGlobalData", () => {
it("should generate the correct response type", () => {
const mockData = {
Query1: {
ENTITY_TYPE: ENTITY_TYPE.ACTION,
},
JSObject1: {
ENTITY_TYPE: ENTITY_TYPE.JSACTION,
myFun1: "function() {}",
myFun2: "async function() {}",
test: "async function() {}",
body: "Some body text",
actionId: "action-id",
},
appsmith: {
ENTITY_TYPE: ENTITY_TYPE.APPSMITH,
URL: { pathname: "/test/editor/path" },
},
};
const result = generateLintingGlobalData(mockData, LINTER_TYPE.ESLINT);
expect(result.globalData).toEqual({
setTimeout: "readonly",
clearTimeout: "readonly",
console: "readonly",
Query1: "readonly",
JSObject1: "readonly",
appsmith: "readonly",
crypto: "readonly",
forge: "readonly",
moment: "readonly",
_: "readonly",
fetch: "readonly",
});
expect(result.asyncFunctions).toEqual([
"Query1.run",
"JSObject1.myFun2",
"JSObject1.test",
]);
expect(result.editorType).toEqual("TestEditorType");
});
it("should handle an empty input and return default values", () => {
const result = generateLintingGlobalData({}, LINTER_TYPE.ESLINT);
expect(result.globalData).toEqual({
setTimeout: "readonly",
clearTimeout: "readonly",
console: "readonly",
crypto: "readonly",
forge: "readonly",
moment: "readonly",
_: "readonly",
fetch: "readonly",
});
expect(result.asyncFunctions).toEqual([]);
});
it("should handle unsupported ENTITY_TYPE gracefully", () => {
const mockData = {
UnknownEntity: {
ENTITY_TYPE: "UNKNOWN",
},
};
const result = generateLintingGlobalData(mockData, LINTER_TYPE.ESLINT);
expect(result.globalData).toEqual({
setTimeout: "readonly",
clearTimeout: "readonly",
console: "readonly",
UnknownEntity: "readonly",
crypto: "readonly",
forge: "readonly",
moment: "readonly",
_: "readonly",
fetch: "readonly",
});
expect(result.asyncFunctions).toEqual([]);
});
});

View File

@ -1,8 +1,18 @@
import { getScriptType } from "workers/Evaluation/evaluate"; import {
EvaluationScriptType,
getScriptType,
} from "workers/Evaluation/evaluate";
import { CustomLintErrorCode, LINTER_TYPE } from "../constants"; import { CustomLintErrorCode, LINTER_TYPE } from "../constants";
import getLintingErrors from "../utils/getLintingErrors"; import getLintingErrors from "../utils/getLintingErrors";
import { Severity } from "entities/AppsmithConsole"; import { Severity } from "entities/AppsmithConsole";
// Define all the custom eslint rules you want to test here
jest.mock("ee/utils/lintRulesHelpers", () => ({
getLintRulesBasedOnContext: jest.fn(() => ({
"customRules/no-floating-promises": "error",
})),
}));
const webworkerTelemetry = {}; const webworkerTelemetry = {};
const linterTypes = [ const linterTypes = [
@ -598,3 +608,72 @@ describe.each(linterTypes)(
}); });
}, },
); );
describe("Custom lint checks", () => {
// This is done since all custom lint rules need eslint as linter type
const getLinterTypeFn = () => LINTER_TYPE.ESLINT;
// Test for 'no floating promises lint rule'
it("1. should show error for unhandled promises", () => {
const data = {
ARGUMENTS: undefined,
Query1: {
actionId: "671b2fcc-e574",
isLoading: false,
responseMeta: {
isExecutionSuccess: false,
},
config: {
timeoutInMillisecond: 10000,
paginationType: "NONE",
encodeParamsToggle: true,
body: "SELECT * FROM <<your_table_name>> LIMIT 10;\n\n-- Please enter a valid table name and hit RUN\n",
pluginSpecifiedTemplates: [
{
value: true,
},
],
},
ENTITY_TYPE: "ACTION",
datasourceUrl: "",
name: "Query1",
run: async function () {},
clear: async function () {},
},
JSObject1: {
body: "export default {\n\tasync handledAsync() {\n\t\tawait Query1.run(); \n\t},\n\tasync unhandledAsync() {\n\t\tQuery1.run();\n\t}\n}",
ENTITY_TYPE: "JSACTION",
actionId: "d24fc04a-910b",
handledAsync: "async function () {\n await Query1.run();\n}",
"handledAsync.data": {},
unhandledAsync: "async function () {\n Query1.run();\n}",
"unhandledAsync.data": {},
},
THIS_CONTEXT: {},
};
const originalBinding = "async unhandledAsync() {\n\t\tQuery1.run();\n\t}";
const script =
"\n function $$closedFn () {\n const $$result = {async unhandledAsync() {\n\t\tQuery1.run();\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n ";
const options = { isJsObject: true };
const lintErrors = getLintingErrors({
getLinterTypeFn,
data,
originalBinding,
script,
scriptType: EvaluationScriptType.EXPRESSION,
options,
webworkerTelemetry,
});
expect(Array.isArray(lintErrors)).toBe(true);
expect(lintErrors.length).toEqual(1);
//expect(lintErrors[0].code).toEqual(
// CustomLintErrorCode.INVALID_ENTITY_PROPERTY,
//);
expect(lintErrors[0].errorMessage.message).toContain(
"Unhandled Promise detected.",
);
});
});

View File

@ -42,6 +42,10 @@ import { profileFn } from "instrumentation/generateWebWorkerTraces";
import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv"; import { WorkerEnv } from "workers/Evaluation/handlers/workerEnv";
import { FEATURE_FLAG } from "ee/entities/FeatureFlag"; import { FEATURE_FLAG } from "ee/entities/FeatureFlag";
import { Linter } from "eslint-linter-browserify"; import { Linter } from "eslint-linter-browserify";
import { ENTITY_TYPE } from "entities/DataTree/dataTreeFactory";
import type { DataTreeEntity } from "entities/DataTree/dataTreeTypes";
import { EditorNames, getEditorType } from "ee/hooks";
import type { AppsmithEntity } from "ee/entities/DataTree/types";
const EvaluationScriptPositions: Record<string, Position> = {}; const EvaluationScriptPositions: Record<string, Position> = {};
@ -71,25 +75,88 @@ function getEvaluationScriptPosition(scriptType: EvaluationScriptType) {
return EvaluationScriptPositions[scriptType]; return EvaluationScriptPositions[scriptType];
} }
function generateLintingGlobalData(data: Record<string, unknown>) { export function generateLintingGlobalData(
const globalData: Record<string, boolean> = {}; data: Record<string, unknown>,
linterType = LINTER_TYPE.JSHINT,
) {
const asyncFunctions: string[] = [];
let editorType = EditorNames.APPLICATION;
let globalData: Record<string, boolean | "readonly" | "writable"> = {};
for (const dataKey in data) { if (linterType === LINTER_TYPE.JSHINT) {
globalData[dataKey] = true; // TODO: cleanup jshint implementation once rollout is complete
for (const dataKey in data) {
globalData[dataKey] = true;
}
// Add all js libraries
const libAccessors = ([] as string[]).concat(
...JSLibraries.map((lib) => lib.accessor),
);
libAccessors.forEach((accessor) => (globalData[accessor] = true));
// Add all supported web apis
objectKeys(SUPPORTED_WEB_APIS).forEach(
(apiName) => (globalData[apiName] = true),
);
} else {
globalData = {
setTimeout: "readonly",
clearTimeout: "readonly",
console: "readonly",
};
for (const dataKey in data) {
globalData[dataKey] = "readonly";
const dataValue = data[dataKey] as DataTreeEntity;
if (
!!dataValue &&
dataValue.hasOwnProperty("ENTITY_TYPE") &&
!!dataValue["ENTITY_TYPE"]
) {
const { ENTITY_TYPE: dataValueEntityType } = dataValue;
if (dataValueEntityType === ENTITY_TYPE.ACTION) {
asyncFunctions.push(`${dataKey}.run`);
} else if (dataValueEntityType === ENTITY_TYPE.JSACTION) {
const ignoreKeys = ["body", "ENTITY_TYPE", "actionId"];
for (const key in dataValue) {
if (!ignoreKeys.includes(key)) {
const value = dataValue[key];
if (
typeof value === "string" &&
value.startsWith("async function")
) {
asyncFunctions.push(`${dataKey}.${key}`);
}
}
}
} else if (dataValueEntityType === ENTITY_TYPE.APPSMITH) {
const appsmithEntity: AppsmithEntity = dataValue;
editorType = getEditorType(appsmithEntity.URL.pathname);
}
}
}
// Add all js libraries
const libAccessors = ([] as string[]).concat(
...JSLibraries.map((lib) => lib.accessor),
);
libAccessors.forEach((accessor) => (globalData[accessor] = "readonly"));
// Add all supported web apis
objectKeys(SUPPORTED_WEB_APIS).forEach(
(apiName) => (globalData[apiName] = "readonly"),
);
} }
// Add all js libraries return { globalData, asyncFunctions, editorType };
const libAccessors = ([] as string[]).concat(
...JSLibraries.map((lib) => lib.accessor),
);
libAccessors.forEach((accessor) => (globalData[accessor] = true));
// Add all supported web apis
objectKeys(SUPPORTED_WEB_APIS).forEach(
(apiName) => (globalData[apiName] = true),
);
return globalData;
} }
function sanitizeESLintErrors( function sanitizeESLintErrors(
@ -302,8 +369,17 @@ export default function getLintingErrors({
}: getLintingErrorsProps): LintError[] { }: getLintingErrorsProps): LintError[] {
const linterType = getLinterTypeFn(); const linterType = getLinterTypeFn();
const scriptPos = getEvaluationScriptPosition(scriptType); const scriptPos = getEvaluationScriptPosition(scriptType);
const lintingGlobalData = generateLintingGlobalData(data); const {
const lintingOptions = lintOptions(lintingGlobalData, linterType); asyncFunctions,
editorType,
globalData: lintingGlobalData,
} = generateLintingGlobalData(data, linterType);
const lintingOptions = lintOptions(
lintingGlobalData,
asyncFunctions,
editorType,
linterType,
);
let messages: Linter.LintMessage[] = []; let messages: Linter.LintMessage[] = [];
let lintErrors: LintError[] = []; let lintErrors: LintError[] = [];

View File

@ -10749,10 +10749,10 @@ __metadata:
languageName: node languageName: node
linkType: hard linkType: hard
"@types/estree@npm:*, @types/estree@npm:1.0.5, @types/estree@npm:^1.0.0": "@types/estree@npm:*, @types/estree@npm:^1.0.0, @types/estree@npm:^1.0.6":
version: 1.0.5 version: 1.0.6
resolution: "@types/estree@npm:1.0.5" resolution: "@types/estree@npm:1.0.6"
checksum: dd8b5bed28e6213b7acd0fb665a84e693554d850b0df423ac8076cc3ad5823a6bc26b0251d080bdc545af83179ede51dd3f6fa78cad2c46ed1f29624ddf3e41a checksum: 8825d6e729e16445d9a1dd2fb1db2edc5ed400799064cd4d028150701031af012ba30d6d03fe9df40f4d7a437d0de6d2b256020152b7b09bde9f2e420afdffd9
languageName: node languageName: node
linkType: hard linkType: hard
@ -10763,6 +10763,13 @@ __metadata:
languageName: node languageName: node
linkType: hard linkType: hard
"@types/estree@npm:1.0.5":
version: 1.0.5
resolution: "@types/estree@npm:1.0.5"
checksum: dd8b5bed28e6213b7acd0fb665a84e693554d850b0df423ac8076cc3ad5823a6bc26b0251d080bdc545af83179ede51dd3f6fa78cad2c46ed1f29624ddf3e41a
languageName: node
linkType: hard
"@types/estree@npm:^0.0.51": "@types/estree@npm:^0.0.51":
version: 0.0.51 version: 0.0.51
resolution: "@types/estree@npm:0.0.51" resolution: "@types/estree@npm:0.0.51"
@ -13179,6 +13186,7 @@ __metadata:
"@types/deep-diff": ^1.0.0 "@types/deep-diff": ^1.0.0
"@types/dom-view-transitions": ^1.0.5 "@types/dom-view-transitions": ^1.0.5
"@types/downloadjs": ^1.4.2 "@types/downloadjs": ^1.4.2
"@types/estree": ^1.0.6
"@types/google.maps": ^3.51.0 "@types/google.maps": ^3.51.0
"@types/jest": ^29.5.3 "@types/jest": ^29.5.3
"@types/jshint": ^2.12.0 "@types/jshint": ^2.12.0