Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[JS] Migrate eslint to latest #13825

Merged
merged 6 commits into from
Apr 15, 2024
Merged

[JS] Migrate eslint to latest #13825

merged 6 commits into from
Apr 15, 2024

Conversation

harsha509
Copy link
Member

@harsha509 harsha509 commented Apr 15, 2024

User description

Eslint migration to 9.x.x

Description

Motivation and Context

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

Type

enhancement


Description

  • Migrated ESLint to version 9.0.0, updating configuration and dependencies.
  • Removed old ESLint configuration and introduced a new setup with updated rules and plugins.
  • Updated Bazel targets to align with the new ESLint setup.
  • Updated package.json, package-lock.json, and pnpm-lock.yaml to reflect the migration to ESLint 9.0.0 and the addition of new ESLint plugins.

Changes walkthrough

Relevant files
Configuration changes
.eslintrc.js
Remove Old ESLint Configuration                                                   

javascript/node/selenium-webdriver/.eslintrc.js

  • Removed the entire ESLint configuration for migration to a new setup.
  • +0/-44   
    eslint.config.js
    Introduce New ESLint Configuration                                             

    javascript/node/selenium-webdriver/eslint.config.js

  • Introduced a new ESLint configuration file with updated dependencies
    and rules.
  • +84/-0   
    BUILD.bazel
    Update Bazel Targets for ESLint Changes                                   

    javascript/node/selenium-webdriver/BUILD.bazel

  • Updated Bazel targets to reflect changes in ESLint configuration and
    dependencies.
  • +4/-12   
    Dependencies
    package.json
    Update ESLint and Plugins in package.json                               

    javascript/node/selenium-webdriver/package.json

    • Updated devDependencies with new ESLint and related plugins.
    +5/-3     
    package-lock.json
    Update package-lock.json for ESLint Migration                       

    package-lock.json

  • Updated package-lock.json to reflect new ESLint version and
    dependencies.
  • +96/-76 
    pnpm-lock.yaml
    Update pnpm-lock.yaml for ESLint Migration                             

    pnpm-lock.yaml

    • Updated pnpm-lock.yaml with new ESLint version and dependencies.
    +49/-34 

    PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @harsha509 harsha509 marked this pull request as ready for review April 15, 2024 18:27
    Copy link
    Contributor

    PR Description updated to latest commit (f9360a4)

    Copy link
    Contributor

    PR Review

    ⏱️ Estimated effort to review [1-5]

    2, because the PR involves updating ESLint to a newer version, modifying configuration files, and adjusting code to adhere to new linting rules. The changes are straightforward and mostly involve syntax and configuration updates rather than complex logic changes.

    🧪 Relevant tests

    No

    🔍 Possible issues

    Deprecated API Usage: The PR removes comments related to deprecated API usage but does not confirm if the underlying code was updated to use the current APIs. This could potentially lead to runtime errors if the deprecated APIs are completely removed in future Node.js versions.

    ESLint Rule Overrides: Several files have ESLint rules disabled for specific lines or blocks of code. While this may be necessary, it's important to ensure that these exceptions do not hide potential bugs or lead to inconsistent code quality.

    🔒 Security concerns

    No


    ✨ Review tool usage guide:

    Overview:
    The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

    The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

    • When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
    /review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
    
    [pr_reviewer]
    some_config1=...
    some_config2=...
    

    See the review usage page for a comprehensive guide on using this tool.

    Copy link
    Contributor

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    Enhancement
    Implement method bodies or throw NotImplementedError.

    Implement the body of the isMultiple, getOptions, getAllSelectedOptions, and
    getFirstSelectedOption methods or throw a NotImplementedError to avoid silent failures or
    confusion during runtime.

    javascript/node/selenium-webdriver/lib/select.js [54-70]

    -isMultiple() {}
    -getOptions() {}
    -getAllSelectedOptions() {}
    -getFirstSelectedOption() {}
    +isMultiple() { throw new Error('NotImplementedError'); }
    +getOptions() { throw new Error('NotImplementedError'); }
    +getAllSelectedOptions() { throw new Error('NotImplementedError'); }
    +getFirstSelectedOption() { throw new Error('NotImplementedError'); }
     
    Align the versions of "globals" used across dependencies to avoid conflicts.

    The addition of "eslint-plugin-mocha" introduces a new version of "globals" as a
    dependency, which might lead to version conflicts or unexpected behavior due to multiple
    versions of "globals" in the dependency tree. Consider aligning the versions of "globals"
    used across different plugins to ensure compatibility.

    package-lock.json [5157]

     "eslint-plugin-mocha": {
    -  "globals": "^13.24.0"
    +  "globals": "^15.0.0"
     }
     
    Remove duplicate eslint dependency to avoid conflicts.

    It seems that @eslint/js and eslint are both specified as dependencies with the same
    version. If @eslint/js is meant to replace eslint, consider removing the eslint entry to
    avoid potential conflicts or confusion about which package should be used.

    pnpm-lock.yaml [118-123]

     '@eslint/js':
       specifier: ^9.0.0
       version: 9.0.0
    -eslint:
    -  specifier: ^9.0.0
    -  version: 9.0.0
     
    Ensure lint scripts cover all project needs or reintroduce specific commands.

    The new lint scripts simplify the command but remove specific ignore patterns and file
    extensions. Ensure that the new configuration adequately covers the project's needs, or
    consider reintroducing more specific linting commands if necessary.

    javascript/node/selenium-webdriver/package.json [48-49]

    -"lint": "eslint .",
    -"lint:fix": "eslint . --fix",
    +"lint": "eslint --ignore-pattern node_modules --ignore-pattern generator --ext js lib/http.js \"**/*.js\"",
    +"lint:fix": "eslint --ignore-pattern node_modules --ignore-pattern generator --ext js lib/http.js \"**/*.js\" --fix",
     
    Best practice
    Avoid broad disabling of no-unused-vars without justification.

    Replace the inline comment disabling the no-unused-vars rule with a more targeted approach
    or justify its necessity, as broad disabling can hide potential issues in the code.

    javascript/node/selenium-webdriver/lib/http.js [475]

    -/*eslint no-unused-vars: "off"*/
    +// Reason for disabling no-unused-vars here...
     
    Justify or avoid disabling no-unused-vars in catch blocks.

    Instead of disabling no-unused-vars, consider using the variable in a meaningful way or if
    the catch block is intentionally left empty, document the reason inline.

    javascript/node/selenium-webdriver/io/index.js [222]

    -/*eslint no-unused-vars: "off"*/
    +// Catch block is intentionally empty because...
     
    Verify "rambda" version compatibility with the project.

    The dependency "rambda" is added with version "7.5.0". Ensure that this version of
    "rambda" is compatible with the rest of your project's dependencies and does not introduce
    any breaking changes, especially if you are migrating from Ramda or another functional
    library.

    package-lock.json [7805]

     "rambda": {
    -  "version": "7.5.0"
    +  "version": "7.4.0"  # Assuming 7.4.0 is the last known compatible version
     }
     
    Maintainability
    Clarify or remove the ESLint directive for no-unused-vars.

    Remove the unnecessary ESLint directive for no-unused-vars if the variable is indeed used
    or provide a comment explaining why it's needed.

    javascript/node/selenium-webdriver/lib/util.js [41]

    -/*eslint no-unused-vars: "off"*/
    +// Explanation for disabling no-unused-vars here...
     
    Possible issue
    Correct the package name for ESLint in "devDependencies".

    It seems that the package "@eslint/js" was added as a new dependency under
    "devDependencies" but with a potentially incorrect package name. The official ESLint
    package is named "eslint", not "@eslint/js". Consider correcting the package name to
    ensure proper installation and functionality of ESLint.

    package-lock.json [12-13]

     "devDependencies": {
    -  "@eslint/js": "^9.0.0"
    +  "eslint": "^9.0.0"
     }
     
    Consider re-adding eslint-plugin-node if Node.js-specific linting rules are needed.

    The eslint-plugin-node dependency has been removed, but if your project relies on
    Node.js-specific linting rules provided by this plugin, consider re-adding it or ensuring
    that its rules are covered by another plugin or configuration.

    javascript/node/selenium-webdriver/package.json [36]

     "eslint-plugin-no-only-tests": "^3.1.0",
    +"eslint-plugin-node": "^11.1.0",
     
    Re-add eslint-plugin-node to eslint-test if needed for Node.js-specific linting.

    The removal of eslint-plugin-node from the eslint-test rule might affect linting for
    Node.js-specific code. If this plugin provided essential rules for your project, consider
    re-adding it or ensuring its rules are covered elsewhere.

    javascript/node/selenium-webdriver/BUILD.bazel [196]

    +":node_modules/eslint-plugin-node",
     ":node_modules/eslint-plugin-prettier",
     
    Verify the new eslint config file is correctly set up.

    The eslint-config target's source file has been changed from .eslintrc.js to
    eslint.config.js. Ensure that this new configuration file exists and is correctly
    configured to maintain the project's coding standards and linting rules.

    javascript/node/selenium-webdriver/BUILD.bazel [211]

    -srcs = ["eslint.config.js"],
    +srcs = [".eslintrc.js"], # or ensure eslint.config.js is correctly set up
     
    Compatibility
    Ensure "eslint-utils" compatibility with the intended Node.js versions.

    The "eslint-utils" package is added with a version that might not be compatible with all
    the versions of Node.js supported by your project. It specifies "node": "^10.0.0 ||
    ^12.0.0 || >= 14.0.0", which excludes Node.js versions below 10. Consider verifying the
    Node.js versions you intend to support and adjust the "eslint-utils" version accordingly.

    package-lock.json [5176]

     "eslint-utils": {
    -  "node": "^10.0.0 || ^12.0.0 || >= 14.0.0"
    +  "node": ">=8.10.0"
     }
     
    Adjust "typescript" version for broader Node.js compatibility.

    The "typescript" package version was updated to "5.4.5", which requires Node.js version
    ">=14.17". If your project needs to support older versions of Node.js, consider using a
    version of "typescript" that is compatible with those older Node.js versions.

    package-lock.json [8745]

     "typescript": {
    -  "version": "5.4.5",
    +  "version": "4.9.5",  # Assuming 4.9.5 is compatible with older Node.js versions
       "engines": {
    -    "node": ">=14.17"
    +    "node": ">=10.0.0"  # Adjust as per your project's requirement
       }
     }
     

    ✨ Improve tool usage guide:

    Overview:
    The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.

    • When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
    /improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
    
    [pr_code_suggestions]
    some_config1=...
    some_config2=...
    

    See the improve usage page for a comprehensive guide on using this tool.

    # for free to join this conversation on GitHub. Already have an account? # to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant