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

FED-194 Fully implement hooks exhaustive dependencies diagnostic, add tests #788

Merged
merged 206 commits into from
Dec 1, 2022

Conversation

greglittlefield-wf
Copy link
Contributor

@greglittlefield-wf greglittlefield-wf commented Oct 18, 2022

Motivation

The useEffect, useMemo, and useCallback hooks take a dependencies array as an argument, but it's easy to make mistakes when declaring those dependencies, and those mistakes can lead to bugs in those components that are often hard to catch and difficult to diagnose/debug, especially for those new to using hooks.

See more info about these hooks and dependencies in the React docs.

To help ensure these dependencies are valid, React recommends the use of an ESLint rule:

We recommend using the exhaustive-deps rule as part of our eslint-plugin-react-hooks package. It warns when dependencies are specified incorrectly and suggests a fix.

But, unfortunately, Dart consumers of React can't take advantage of that rule.

Changes

Fully implement and test the HooksExhaustiveDeps diagnostic.

Ported/forked from the JS eslint-plugin-react-hooks rule, this new diagnostic aims to provide parity with the dev experience of that lint, with some behavior altered to accommodate differences between JavaScript, Dart, and their respective React APIs.

HooksExhaustiveDeps changes:

  • Finish port/implementation
    • Make null-safe
    • Fix and fully implement dependency logic for useState/useReducer hooks
    • Fix dependency logic for function props
    • Fix treatment of property accesses on dependencies
    • Handle edge cases involving cascade operator
    • Treat generic parameters as dependencies (since Dart uses reified generics)
    • Refactor to allow async diagnosticCollector.addErrorWithFix calls
    • Finish porting misc. unported logic
    • Address misc. fixmes
    • Fix misc. mistakes when porting that were caught by manual testing/tests
  • Add tests
    • Port tests from JS implementation
    • Add new Dart-specific tests and regression tests
  • Enable by default :)

Other changes to support this:

  • AnalysisOptionsReader
    • Move field declaration to from DiagnosticMixin plugin so that it can be used in getDiagnosticContributors (and potentially other places outside of diagnostics)
    • Add getOptionsForContextRoot so it can be used in places where we don't have a ResolvedUnitResult (like getDiagnosticContributors)
    • Rename to PluginOptionsReader so it wouldn't be confused with similarly-named analyzer APIs
  • New utilities:
    • ast_util.dart
      • lookUpFunction
      • lookUpDeclaration
      • lookUpParameter
      • getNonCascadedPropertyBeingAccessed
      • PropertyInvocation
      • findReferences
    • prettyPrint
    • List.elementAtOrNull
    • WeakMap
    • DiagnosticTestBase.expectNoErrors
  • Use a single implementation for debug comments, document in README
  • Add diagnostic metrics capturing when using debug:over_react_metrics flag
  • Moved code for dart:mirrors-based constant instantiation out of ast_util.dart, since it seemed different enough, that file was getting pretty big, and is currently only used for docs generation

Release Notes

  • Add the HooksExhaustiveDeps diagnostic. Ported/forked from the JS eslint-plugin-react-hooks rule, this new diagnostic aims to provide parity with the dev experience of that lint, with some behavior altered to accommodate differences between JavaScript, Dart, and their respective React APIs.

Review

See CONTRIBUTING.md for more details on review types (+1 / QA +1 / +10) and code review process.

Please review:

QA Checklist

  • Tests were updated and provide good coverage of the changeset and other affected code
  • Manual testing was performed if needed

Merge Checklist

While we perform many automated checks before auto-merging, some manual checks are needed:

  • A Frontend Frameworks Design member has reviewed these changes
  • There are no unaddressed comments - this check can be automated if reviewers use the "Request Changes" feature
  • For release PRs - Version metadata in Rosie comment is correct

@greglittlefield-wf greglittlefield-wf marked this pull request as ready for review October 19, 2022 00:32
Copy link
Contributor

@kealjones-wk kealjones-wk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+10

@greglittlefield-wf
Copy link
Contributor Author

@Workiva/release-management-p

Copy link

@rmconsole-wf rmconsole-wf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from RM

@rmconsole5-wk rmconsole5-wk merged commit 010a377 into master Dec 1, 2022
@rmconsole6-wk rmconsole6-wk deleted the exhaustive-deps-tests branch December 1, 2022 23:43
@kealjones-wk kealjones-wk removed their assignment Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants