chore: add eslint package to handle linting #37417
Conversation
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (3)
16-72: Add test cases for API error scenarios.While the happy path tests for supported APIs are good, consider adding test cases for error scenarios (e.g., failed fetch, rejected promises).
Example test case:
it("should handle fetch API errors", () => { const originalBinding = "{{fetch().catch(e => e)}}"; const script = "fetch().catch(e => e)"; const scriptType = getScriptType(false, true); const lintErrors = getLintingErrors({ getLinterTypeFn: () => linterType, data, originalBinding, script, scriptType, webworkerTelemetry, }); expect(lintErrors.length).toEqual(0); });
130-215: Add test case for deep mutation attempts.The mutation tests are good but could be enhanced by testing deep mutation attempts on nested objects.
Example test:
it("4. Deep mutation attempts on nested objects", () => { const data = { appsmith: { store: { nested: { value: 42 } } } }; const originalBinding = "myFun1() {\n\t\tappsmith.store.nested.value = 43;\n\t}"; // ... rest of the test setup });
218-566: Consider grouping related config tests.The config rule tests could be better organized by grouping related rules together (e.g., all syntax-related rules, all security-related rules).
Example structure:
describe("4. Config rule tests", () => { describe("4.1 Syntax rules", () => { // Tests for curly, semicolon, etc. }); describe("4.2 Security rules", () => { // Tests for eval, freeze, etc. }); describe("4.3 Variable usage rules", () => { // Tests for unused, undef, etc. }); });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts(1 hunks)
🔇 Additional comments (2)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (2)
8-11: LGTM! Good use of parameterized testing.
The linter types array setup enables efficient testing of both JSHint and ESLint configurations.
474-499: Add test case for indirect eval usage.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (2)
144-165: Consider adding more edge cases for mutation tests.While the current test covers basic mutation scenarios, consider adding tests for:
- Nested property mutations
- Array mutations
- Object method mutations
// Example test case for nested property mutation it("4. Mutating nested properties", () => { const originalBinding = "myFun1() {\n\t\tInput1.nested.prop = \"\";\n\t}"; const script = "\n function $$closedFn () {\n const $$result = {myFun1() {\n\t\tInput1.nested.prop = \"\";\n\t}}\n return $$result\n }\n $$closedFn.call(THIS_CONTEXT)\n"; // ... rest of the test });
529-548: Consider adding dynamic property access tests.While the current test covers basic bracket notation, consider adding tests for:
- Dynamic property names
- Symbol properties
- Computed property names
it("13b. Should handle dynamic property access", () => { const data = { obj: { [Symbol.for('test')]: 1 } }; const propName = 'dynamic'; const originalBinding = `{{ obj[${propName}]; }}`; // ... rest of the test });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (3)
app/client/package.json(1 hunks)app/client/src/plugins/Linting/tests/getLintingErrors.test.ts(1 hunks)app/client/test/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/client/package.json
- app/client/test/setup.ts
🔇 Additional comments (5)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (5)
8-11: LGTM! Well-structured linter configuration.
The array of linter types is well-organized and allows for easy extension if more linter types need to be added in the future.
13-15: Good use of parameterized testing!
Using describe.each is an excellent approach to avoid test code duplication while testing multiple linter types.
218-239: Consider adding strict equality tests.
The current test only verifies loose equality (==). Add test cases for strict equality (===) to ensure comprehensive coverage.
474-499: Add test for indirect eval usage.
The current test only covers direct eval usage. Consider adding tests for indirect eval scenarios.
1-574: Verify test coverage metrics.
The test suite appears comprehensive, but let's verify the coverage metrics to ensure we haven't missed any edge cases.
✅ Verification successful
Based on the shell script results, I can now verify the test coverage comprehensively. Let me generate the final response:
Test coverage is comprehensive and well-structured
The test suite adequately covers:
- All custom lint error codes defined in the codebase (INVALID_ENTITY_PROPERTY, ASYNC_FUNCTION_BOUND_TO_SYNC_FIELD, INVALID_WIDGET_PROPERTY_SETTER, INVALID_APPSMITH_STORE_PROPERTY_SETTER, ACTION_MODAL_STRING, INVALID_INPUTS)
- Multiple linter configurations (JSHint and ESLint)
- Edge cases for window APIs, mutations, and various JavaScript patterns
- Comprehensive config rule validations including eqeqeq, curly, freeze, undef, forin, noempty, strict, unused, boss, evil, funcscope, sub, and expr
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all linting-related files to ensure test coverage
fd -e ts -e js --full-path '.*Linting.*' | grep -v 'test'
# Find all custom lint error codes to ensure they're tested
rg -g '*.ts' 'CustomLintErrorCode\.' --no-heading
Length of output: 5853
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (4)
8-11: Add TypeScript type definitions for linterTypes array.Consider adding an interface or type definition for the linter configuration objects.
+interface LinterConfig { + linterType: typeof LINTER_TYPE[keyof typeof LINTER_TYPE]; + name: string; +} + -const linterTypes = [ +const linterTypes: LinterConfig[] = [ { linterType: LINTER_TYPE.JSHINT, name: "JSHint" }, { linterType: LINTER_TYPE.ESLINT, name: "ESLint" }, ];
16-128: Add edge cases for API testing.Consider adding test cases for:
- Nested API calls (e.g.,
setTimeout(() => fetch(), 1000))- Method chaining (e.g.,
console.log().toString())
130-215: Enhance error message assertions.Consider extracting expected error messages into constants and adding assertions for error line numbers and columns.
+const ERROR_MESSAGES = { + INVALID_ENTITY_PROPERTY: "Cannot modify widget properties directly", + INVALID_STORE_PROPERTY: "Cannot modify store properties directly", +} as const; expect(lintErrors[0].code).toEqual(CustomLintErrorCode.INVALID_ENTITY_PROPERTY); +expect(lintErrors[0].errorMessage.message).toBe(ERROR_MESSAGES.INVALID_ENTITY_PROPERTY); +expect(lintErrors[0].errorMessage.line).toBe(4);
218-597: Improve test descriptions for clarity.Some test descriptions could be more explicit about what they're testing. For example:
- "Should allow '==' and '!=' without errors" could be "Should allow loose equality operators (== and !=) when eqeqeq is false"
- "Should allow blocks without brackets" could be "Should allow single-line blocks without curly braces when curly is false"
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts(1 hunks)
🔇 Additional comments (1)
app/client/src/plugins/Linting/tests/getLintingErrors.test.ts (1)
1-600: LGTM! Comprehensive test coverage.
The test suite effectively covers both linter implementations with good edge cases and error scenarios.
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
app/client/src/plugins/Linting/utils/getLintingErrors.ts (2)
95-147: Consider extracting shared line number adjustment logicThe line number adjustment logic is duplicated between
sanitizeESLintErrorsandsanitizeJSHintErrors. Consider extracting this into a shared utility function.+function adjustLineNumbers(message: string, scriptPos: Position): { + message: string, + isInvalidErrorMessage: boolean +} { + const matchedLines = message.match(/line \d/gi); + const lineNumbersInErrorMessage = new Set<number>(); + let isInvalidErrorMessage = false; + + if (matchedLines) { + matchedLines.forEach((lineStatement) => { + const digitString = lineStatement.split(" ")[1]; + const digit = Number(digitString); + + if (isNumber(digit)) { + if (digit < scriptPos.line) { + isInvalidErrorMessage = true; + } else { + lineNumbersInErrorMessage.add(digit); + } + } + }); + } + + if (lineNumbersInErrorMessage.size) { + Array.from(lineNumbersInErrorMessage).forEach((lineNumber) => { + message = message.replaceAll( + `line ${lineNumber}`, + `line ${lineNumber - scriptPos.line + 1}`, + ); + }); + } + + return { message, isInvalidErrorMessage }; +}
219-251: Add JSDoc documentation for better maintainabilityConsider adding JSDoc documentation to describe the function's purpose, parameters, and return type.
+/** + * Converts ESLint error messages to Appsmith's internal lint error format + * @param eslintError - The ESLint error message + * @param script - The script being linted + * @param originalBinding - The original binding expression + * @param scriptPos - The position of the script in the template + * @returns LintError - The converted lint error in Appsmith's format + */ function convertESLintErrorToAppsmithLintError(
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
app/client/src/plugins/Linting/utils/getLintingErrors.ts(6 hunks)app/client/test/setup.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/test/setup.ts
🔇 Additional comments (2)
app/client/src/plugins/Linting/utils/getLintingErrors.ts (2)
Line range hint 295-367: Verify feature flag implementation
The code uses FEATURE_FLAG.rollout_eslint_enabled for the ESLint rollout. Let's verify this flag is properly defined.
✅ Verification successful
Feature flag implementation is correctly defined and used
The feature flag rollout_eslint_enabled is properly defined in app/client/src/ce/entities/FeatureFlag.ts with a default value of false, and it's correctly used in the linting implementation to toggle between ESLint and JSHint.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check feature flag definition
rg -A 2 "rollout_eslint_enabled" --type ts
Length of output: 833
44-44: Verify eslint-linter-browserify package dependency
Let's ensure the package is properly added to the project dependencies.
✅ Verification successful
Package dependency verified successfully
The eslint-linter-browserify package is correctly added as a dependency with version ^9.15.0.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if eslint-linter-browserify is in package.json
cat package.json | jq '.dependencies["eslint-linter-browserify"] // .devDependencies["eslint-linter-browserify"]'
Length of output: 160
Script:
#!/bin/bash
# Let's try finding the package.json files in the repository
fd package.json
# Once we find the package.json files, let's check their content for eslint-linter-browserify
fd package.json --exec cat {} \; | grep -i "eslint-linter-browserify"
Length of output: 844
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/client/packages/ast/package.json (1)
19-20: Consider maintaining version flexibilityThe removal of the caret (^) for both dependencies enforces strict versions. While this ensures stability, it might complicate future updates. Was this intentional?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/client/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (2)
app/client/packages/ast/package.json(1 hunks)app/client/packages/ast/src/actionCreator/index.ts(0 hunks)
💤 Files with no reviewable changes (1)
- app/client/packages/ast/src/actionCreator/index.ts
🔇 Additional comments (1)
app/client/packages/ast/package.json (1)
19-20: Verify compatibility with ESLint integration
Since this PR transitions to ESLint, let's ensure these specific versions of acorn and acorn-walk are compatible with the ESLint version being introduced.
✅ Verification successful
Let me analyze ESLint's configuration and parser setup to ensure compatibility with these acorn versions.
Let me check ESLint plugin's dependencies and usage of acorn to verify compatibility.
Version changes are safe for ESLint integration
The pinned versions of acorn (8.14.0) and acorn-walk (8.3.4) in the ast package are compatible with the codebase. The ESLint plugin doesn't directly depend on acorn, and these versions are within the same major version (8.x.x) as the ones used in the client app (8.10.0), maintaining backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check ESLint's package.json for its acorn dependency version
rg -A 5 '"acorn":|"acorn-walk":' $(fd -t f package.json)
Length of output: 1570
Script:
#!/bin/bash
# Check ESLint's dependencies and version
rg '"eslint":|"@typescript-eslint":|"parser":' app/client/packages/eslint-plugin/package.json -A 2
# Check if acorn is used as parser in ESLint config
rg -l 'parser.*acorn' $(fd -e js -e json -e ts -e yml -e yaml)
# Look for ESLint configuration files
fd -e js -e json -e yml -e yaml -E '*lock*' -E 'node_modules' | rg -i 'eslint'
Length of output: 119069
Script:
#!/bin/bash
# Check ESLint's package dependencies and version
cat app/client/packages/eslint-plugin/package.json | grep -A 10 '"dependencies":'
# Check if acorn is used in ESLint plugin code
rg -l "acorn" app/client/packages/eslint-plugin/src/
Length of output: 203
|
This PR has increased the number of cyclic dependencies by 1, when compared with the release branch. Refer this document to identify the cyclic dependencies introduced by this PR. |
| * these line numbers need to be re-calculated based on the binding location. | ||
| * Errors referencing line numbers outside the user's script should also be ignored | ||
| * */ | ||
| let message = lintError.message; |
There was a problem hiding this comment.
@ayushpahwa sanitizeESLintErrors function looks very similar to sanitizeJSHintErrors, do you think we can keep a single function but pass in linterType instead?
There was a problem hiding this comment.
The idea is to remove the jshint functions completely. If i add a conditional, this just makes the removal process complicated.
| @@ -1,38 +1,85 @@ | |||
| import { ECMA_VERSION } from "@shared/ast"; | |||
| import type { LintOptions } from "jshint"; | |||
| import isEntityFunction from "./utils/isEntityFunction"; | |||
| import type { Linter } from "eslint-linter-browserify"; | |||
There was a problem hiding this comment.
@ayushpahwa Where have we defined the flag that enables ESLint?
There was a problem hiding this comment.
## Description We are shifting the linter engine for jshint to eslint. This PR adds the package needed for the same and also adds the required configs. The eslint feature is behing a rollout feature flag, it's currently at 0% right now. > [!NOTE] > This PR is part of a series of [stacked diffs](https://newsletter.pragmaticengineer.com/p/stacked-diffs) and might have changes from it's parent PRs. Untill the blocking parent PRs are merged, this diff would show more changes than are relevant for this PR. Blocking PRs: - appsmithorg#36548 Fixes appsmithorg#37254 ## Automation /test js sanity ### 🔍 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/11965021215> > Commit: 579e477 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11965021215&attempt=1" target="_blank">Cypress dashboard</a>. > Tags: `@tag.JS, @tag.Sanity` > Spec: > <hr>Fri, 22 Nov 2024 02:49:47 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Integrated ESLint support alongside existing JSHint functionality for enhanced linting capabilities. - Added a new dependency for ESLint linting. - **Improvements** - Enhanced error handling and reporting for linting errors, ensuring consistency across different linting tools. - Updated testing framework to accommodate multiple linter types without duplicating logic. - Introduced new functions for sanitizing and converting ESLint errors to the application's format. - Added support for structured cloning in the testing setup. - Improved logic for determining main actions and error handling in conditional expressions. - **Bug Fixes** - Improved logic for handling lint errors, particularly in differentiating between ESLint and JSHint errors. - **Documentation** - Updated comments for clarity on linting processes and configurations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
We are shifting the linter engine for jshint to eslint. This PR adds the package needed for the same and also adds the required configs. The eslint feature is behing a rollout feature flag, it's currently at 0% right now.
Fixes #37254
Automation
/test js sanity
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11965021215
Commit: 579e477
Cypress dashboard.
Tags:
@tag.JS, @tag.SanitySpec:
Fri, 22 Nov 2024 02:49:47 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Documentation