feat: custom eslint rule infra #38345
Conversation
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12479987595. |
|
Deploy-Preview-URL: https://ce-38345.dp.appsmith.com |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/12484477041. |
|
Deploy-Preview-URL: https://ce-38345.dp.appsmith.com |
github-actions
bot
added
Task
ayushpahwa
requested review from
ankitakinger,
KelvinOm,
AmanAgarwal041 and
sneha122
and removed request for
KelvinOm
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
app/client/src/plugins/Linting/customRules/no-floating-promises.ts (1)
80-119: Enhance Handling of Async CallsIt might be worthwhile to also enforce calling
.catch()on async calls to avoid unhandled errors, especially if an error is not handled by.then().app/client/src/plugins/Linting/utils/getLintingErrors.ts (2)
98-101: Avoid Assignments Within Array CallbacksUsing assignments directly in arrow function expressions can be confusing. Consider using a for-loop or separate statements for better readability and maintainability.
- libAccessors.forEach((accessor) => (globalData[accessor] = "readonly")); - objectKeys(SUPPORTED_WEB_APIS).forEach((apiName) => (globalData[apiName] = "readonly")); + for (const accessor of libAccessors) { + globalData[accessor] = "readonly"; + } + for (const apiName of objectKeys(SUPPORTED_WEB_APIS)) { + globalData[apiName] = "readonly"; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 98-98: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
152-155: Refrain From Assignments in ExpressionsReiterating the suggestion above: consider rewriting these array callbacks with simpler loops.
🧰 Tools
🪛 Biome (1.9.4)
[error] 152-152: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
[error] 155-155: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
app/client/src/ce/utils/lintRulesHelpers.ts (1)
10-10: Refactor the Empty Object ParameterIf you’re not using properties from
ContextType, remove or rename the destructured parameter for clarity.-export const getLintRulesBasedOnContext = ({}: ContextType): Record<string, "off" | "error"> => { +export const getLintRulesBasedOnContext = (_: ContextType): Record<string, "off" | "error"> => {🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
app/client/src/ce/hooks/index.ts (1)
21-23: Optional Handling for Unmatched PathsThe usage of
matchPathis straightforward. Consider handling logging or a default case if no routes match.app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts (1)
14-99: Good Coverage, Consider Edge CasesThe tests validate common flows (normal, empty, unsupported). You could add negative or malformed data scenarios for deeper validation.
app/client/src/plugins/Linting/constants.ts (1)
13-15: Signature expansion looks appropriate.
AcceptingasyncFunctionsandeditorTypesupports more dynamic context for linting.To enhance clarity, update the function’s docstring (if available) to describe both new parameters.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
app/client/package.json(1 hunks)app/client/src/ce/hooks/index.ts(2 hunks)app/client/src/ce/utils/lintRulesHelpers.ts(1 hunks)app/client/src/ee/utils/lintRulesHelpers.ts(1 hunks)app/client/src/plugins/Linting/constants.ts(3 hunks)app/client/src/plugins/Linting/customRules/no-floating-promises.ts(1 hunks)app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts(1 hunks)app/client/src/plugins/Linting/tests/getLintingErrors.test.ts(2 hunks)app/client/src/plugins/Linting/utils/getLintingErrors.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/client/src/ee/utils/lintRulesHelpers.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/client/src/ce/utils/lintRulesHelpers.ts
[error] 10-10: Unexpected empty object pattern.
(lint/correctness/noEmptyPattern)
app/client/src/plugins/Linting/utils/getLintingErrors.ts
[error] 98-98: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 101-101: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 117-117: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
[error] 152-152: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 155-155: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (15)
app/client/src/plugins/Linting/customRules/no-floating-promises.ts (1)
4-17: Rule Metadata Is Well-Defined
The metadata and documentation fields look clear and properly structured.
app/client/src/plugins/Linting/utils/getLintingErrors.ts (2)
45-48: No Issues Found
These imports appear necessary and properly placed for the new logic.
372-382: No Issues Found
Integration with generateLintingGlobalData works as intended.
app/client/src/ce/hooks/index.ts (1)
33-36: No Issues Found
The custom hook’s simplicity helps keep logic clear by delegating to getEditorType.
app/client/src/plugins/Linting/tests/generateLintingGlobalData.test.ts (1)
5-12: Mocks Are Well-Implemented
Mocking getEditorType keeps these tests self-contained and ensures consistent behavior.
app/client/src/plugins/Linting/constants.ts (5)
4-5: Good separation of concerns.
The imports for noFloatingPromisesLintRule and getLintRulesBasedOnContext keep the custom logic modular.
45-45: Context-driven rules.
Using getLintRulesBasedOnContext to load rules for the given editorType is flexible and future-proof.
50-57: Great approach to customizing ESLint via settings.
Providing both globalData and asyncFunctions here allows your custom rules to reference them easily.
58-64: Kudos for defining customRules plugin.
Registering "no-floating-promises" under a dedicated plugin helps keep rules organized.
66-66: Ensure consistent overrides.
Merging extraRules with existing ESLint rules is correct. Confirm that none of the existing rules conflict with your custom rules.
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (4)
1-4: Neat import for accurate script type detection.
EvaluationScriptType and getScriptType unify the test logic with production usage.
9-15: Context-based mocking.
Mocking getLintRulesBasedOnContext confirms your new rule loading approach without interfering with other tests.
612-617: Focused test suite.
The new "Custom lint checks" section clarifies your custom ESLint rule coverage.
617-678: Test ensures unhandled Promise detection.
Validates that the custom rule flags an async call without await. This is a key feature for Promise safety.
app/client/package.json (1)
264-264: Type definitions added.
Including @types/estree supports AST-based lint rules effectively. Great addition for type safety.
|
|
||
| if ( | ||
| !!dataValue && | ||
| dataValue.hasOwnProperty("ENTITY_TYPE") && |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use Object.hasOwn Instead of hasOwnProperty
Avoid calling hasOwnProperty directly from the object to prevent prototype pollution issues.
- dataValue.hasOwnProperty("ENTITY_TYPE")
+ Object.hasOwn(dataValue, "ENTITY_TYPE")📝 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.
| dataValue.hasOwnProperty("ENTITY_TYPE") && | |
| Object.hasOwn(dataValue, "ENTITY_TYPE") && |
🧰 Tools
🪛 Biome (1.9.4)
[error] 117-117: Do not access Object.prototype method 'hasOwnProperty' from target object.
It's recommended using Object.hasOwn() instead of using Object.hasOwnProperty().
See MDN web docs for more details.
(lint/suspicious/noPrototypeBuiltins)
| import type { Rule } from "eslint"; | ||
| import type * as ESTree from "estree"; | ||
|
|
||
| export const noFloatingPromisesLintRule: Rule.RuleModule = { |
There was a problem hiding this comment.
We have a package for custom rules. However, I'm not entirely sure that these rules are necessary at all. Why can't we just use this plugin?
There was a problem hiding this comment.
Hey @KelvinOm thanks for pointing out the eslint plugin, I'll try them out. As for the package for custom rules, these are for the developers building the TS code of appsmith. The eslint rule that I added is for the appsmith users who write JS code inside the JSObjects of apps, packages and workflows. If we mix them, there might be confusion related to how the rules are being applied. However we do need to communicate this properly, would updates to readme help here?
There was a problem hiding this comment.
I tried to see if the plugins can work. The issue seems to be in the way we lint our code in appsmith. Each function is linted one at a time with limited scope of the rest of the world. Also most of the async functions like Query.run etc are not part of the defined codebase that gets linted, it needs to be passed down as global context. Hence these out of the box plugins don't fit in this particular case.
Description
This PR introduces the custom ESLint rule to handle floating promises in our JS Objects (using async function calls without await inside async functions). Along with this, some refactors were needed
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 #37255
Automation
/test sanity js
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/12558146123
Commit: 36b6610
Cypress dashboard.
Tags:
@tag.Sanity, @tag.JSSpec:
Tue, 31 Dec 2024 10:54:23 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Development
Chores