Skip to content
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.

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

chore: Enable @typescript-eslint/no-explicit-any as an error rule #23531

Merged
merged 30 commits into from
Mar 27, 2024

Conversation

MajorLift
Copy link
Contributor

@MajorLift MajorLift commented Mar 15, 2024

Description

The ongoing TypeScript conversion effort will be most effective if any usage can be minimized in incoming code. A helpful step in achieving this is to enable @typescript-eslint/no-explicit-any as an error rule.

To minimize disruption to existing code, all 385 existing violations have been annotated with an eslint-disable-next-line directive and TODO comment. Errors for this rule will only be triggered by new instances of any.

This approach has previously been successfully applied to the core monorepo.

Open in GitHub Codespaces

Disclaimer

This is a major change, so please feel free to voice any concerns. Code review and updates to documentation may be sufficient to encourage best practices for the time being.

That said, if minimizing any usage through these other means sounds like a good idea, that might be a strong sign that enabling this linter rule aligns with the coding standards that we already have in place.

Related issues

Manual testing steps

Screenshots/Recordings

Pre-merge author checklist

  • I’ve followed MetaMask Coding Standards.
  • I've clearly explained what problem this PR is solving and how it is solved.
  • I've linked related issues
  • I've included manual testing steps
  • I've included screenshots/recordings if applicable
  • I’ve included tests if applicable
  • I’ve documented my code using JSDoc format if applicable
  • I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.
  • I’ve properly set the pull request status:
    • In case it's not yet "ready for review", I've set it to "draft".
    • In case it's "ready for review", I've changed it from "draft" to "non-draft".

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

This comment was marked as resolved.

Copy link

codecov bot commented Mar 15, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.13%. Comparing base (d405dcc) to head (2976bca).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop   #23531   +/-   ##
========================================
  Coverage    69.13%   69.13%           
========================================
  Files         1160     1160           
  Lines        44260    44260           
  Branches     11839    11839           
========================================
  Hits         30596    30596           
  Misses       13664    13664           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@MajorLift MajorLift self-assigned this Mar 15, 2024
@metamaskbot

This comment was marked as duplicate.

@MajorLift MajorLift marked this pull request as ready for review March 15, 2024 21:28
@MajorLift MajorLift requested review from kumavis and a team as code owners March 15, 2024 21:28
@MajorLift MajorLift requested a review from DDDDDanica March 15, 2024 21:31
@MajorLift MajorLift added contributor experience An issue that impacts, or planned improvement to, the contributor experience. needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) labels Mar 15, 2024
@brad-decker
Copy link
Contributor

@Gudahtt / @mcmire / @davidmurdoch is this how you would suggest handling enabling this rule to prevent future breaks? My concern is the TODO's will get lost. Though the only alternative I know is to ignore rules by file.

@mcmire
Copy link
Contributor

mcmire commented Mar 20, 2024

@brad-decker Sure, good question.

The motivation for enabling this rule is to prevent further uses of any, which (despite the name) completely disables typechecking for the variable in question. We noticed that we had a lot of instances of any on core, and the approach we took to solve it was to add eslint-disable lines. Then, we did a search to count the number of instances of the TODO: Replace `any` with type comment across the project per file. We grouped files by area and then created a ticket on our Zenhub board for each area. Since then we've been working through those tickets to replace any with a better type and remove the eslint-disable comments.

This approach has worked for us, but if there is another approach that you think would work for you, we can certainly follow it.

DDDDDanica
DDDDDanica previously approved these changes Mar 25, 2024
@DDDDDanica DDDDDanica changed the title style: Enable @typescript-eslint/no-explicit-any as an error rule chore: Enable @typescript-eslint/no-explicit-any as an error rule Mar 25, 2024
@metamaskbot

This comment was marked as duplicate.

brad-decker
brad-decker previously approved these changes Mar 26, 2024
@metamaskbot

This comment has been minimized.

@metamaskbot

This comment was marked as duplicate.

Mrtenz
Mrtenz previously approved these changes Mar 27, 2024
@MajorLift MajorLift dismissed stale reviews from Mrtenz, brad-decker, dbrans, and mcmire via c277ce3 March 27, 2024 14:29
@metamaskbot

This comment was marked as duplicate.

@metamaskbot
Copy link
Collaborator

Builds ready [2976bca]
Page Load Metrics (952 ± 504 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint782301374823
domContentLoaded16104422613
load5526519521049504
domInteractive16104422613
Bundle size diffs [🚨 Warning! Bundle size has increased!]
  • background: 29 Bytes (0.00%)
  • ui: 0 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@MajorLift MajorLift merged commit 6ad313f into develop Mar 27, 2024
66 checks passed
@MajorLift MajorLift deleted the 240315-enable-no-explicit-any branch March 27, 2024 20:35
@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2024
@github-actions github-actions bot removed the needs-dev-review PR needs reviews from other engineers (in order to receive required approvals) label Mar 27, 2024
@metamaskbot metamaskbot added the release-11.15.0 Issue or pull request that will be included in release 11.15.0 label Mar 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
contributor experience An issue that impacts, or planned improvement to, the contributor experience. release-11.15.0 Issue or pull request that will be included in release 11.15.0 team-wallet-framework
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

style: Enable @typescript-eslint/no-explicit-any as an error rule
8 participants