chore(website): Enable react-hooks exhaustive deps rules #5663

amorimr · 2022-09-15T17:25:32Z

PR Checklist

Addresses an existing open issue: fixes Repo: Enable react-hooks/exhaustive-deps in website ESLint config #5549
That issue was marked as accepting prs
Steps in CONTRIBUTING.md were taken

Overview

Enables the react-hooks/exhaustive-deps eslint rules in the website package.
Fixes warnings in the package

nx-cloud · 2022-09-15T17:25:33Z

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 507c7a5. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch

✅ Successfully ran 40 targets

Sent with 💌 from NxCloud.

typescript-eslint · 2022-09-15T17:25:36Z

Thanks for the PR, @amorimr!

typescript-eslint is a 100% community driven project, and we are incredibly grateful that you are contributing to that community.

The core maintainers work on this in their personal time, so please understand that it may not be possible for them to review your work immediately.

Thanks again!

🙏 Please, if you or your company is finding typescript-eslint valuable, help us sustain the project by sponsoring it transparently on https://opencollective.com/typescript-eslint. As a thank you, your profile/company logo will be added to our main README which receives thousands of unique visitors per day.

netlify · 2022-09-15T17:25:41Z

✅ Deploy Preview for typescript-eslint ready!

Name	Link
🔨 Latest commit	`507c7a5`
🔍 Latest deploy log	https://app.netlify.com/sites/typescript-eslint/deploys/63339d0210ae8c00073bf332
😎 Deploy Preview	https://deploy-preview-5663--typescript-eslint.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

amorimr · 2022-09-15T17:26:04Z

packages/website/.eslintrc.js

@@ -22,7 +22,6 @@ module.exports = {
    'react/jsx-no-target-blank': 'off',
    'react/no-unescaped-entities': 'off',
    '@typescript-eslint/internal/prefer-ast-types-enum': 'off',
-    'react-hooks/exhaustive-deps': 'off', // TODO: enable it later


Sets the rules to the default behaviour ("warn").

amorimr · 2022-09-15T17:26:32Z

packages/website/src/components/Playground.tsx

+  const onLoaded = useCallback(
+    (ruleNames: RuleDetails[], tsVersions: readonly string[]): void => {
+      setRuleNames(ruleNames);
+      setTSVersion(tsVersions);
+      setIsLoading(false);
+    },
+    [],
+  );
+


This small refactoring was necessary to fix an issue raised by updating one of the dependency arrays on useSandboxServices.ts:

Since the original onLoaded was inlined, it was being recreated at every render, causing racing conditions on useSandboxServices effects that crashed the page.

amorimr · 2022-09-15T17:26:53Z

packages/website/src/components/ast/PropertyName.tsx

@@ -9,21 +9,23 @@ export interface PropertyNameProps {
 }

 export default function PropertyName(props: PropertyNameProps): JSX.Element {
+  const { onClick: onClickProps, onHover } = props;


Destructuring props to avoid stale values as suggested by: facebook/react#16265 (comment)

amorimr · 2022-09-15T17:27:35Z

packages/website/src/components/inputs/Checkbox.tsx

-    if (!checkboxRef.current) {
-      return;
-    }
+  const checkboxRef = useCallback(


Small refactoring: Callback refs are generally the recommended way to interacting with the underlying DOM node:
https://reactjs.org/docs/hooks-faq.html#how-can-i-measure-a-dom-node

JoshuaKGoldberg

Looks good to me, thanks for sending this in! My only note is that it's unfortunate how some of the deps arrays have so many entries. It'd be nice if we could split those up / reduce them a bit.

@armano2 has done a bunch of work on this so I'll defer approval to them if they have time to review. 🙂

…ive-deps

codecov · 2022-09-25T01:42:27Z

Codecov Report

Merging #5663 (507c7a5) into main (84e316b) will increase coverage by 2.81%.
The diff coverage is n/a.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5663      +/-   ##
==========================================
+ Coverage   91.01%   93.82%   +2.81%     
==========================================
  Files         365      134     -231     
  Lines       11962     1506   -10456     
  Branches     3483      226    -3257     
==========================================
- Hits        10887     1413    -9474     
+ Misses        781       60     -721     
+ Partials      294       33     -261

Flag	Coverage Δ
unittest	`93.82% <ø> (+2.81%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/utils/src/eslint-utils/RuleCreator.ts
...-plugin/src/rules/restrict-template-expressions.ts
...ages/eslint-plugin/src/rules/unified-signatures.ts
...plugin/src/rules/explicit-module-boundary-types.ts
...s/eslint-plugin/src/rules/no-implicit-any-catch.ts
...int-plugin/src/rules/strict-boolean-expressions.ts
...es/eslint-plugin/src/rules/prefer-function-type.ts
packages/utils/src/ast-utils/helpers.ts
packages/utils/src/ts-eslint-scope/ScopeManager.ts
.../eslint-plugin/src/rules/prefer-ts-expect-error.ts
... and 221 more

JoshuaKGoldberg

Code looks great, thanks for sending this in! Just blocked on the introduced bug around AST property expansion.

packages/website/src/components/editor/LoadedEditor.tsx

JoshuaKGoldberg · 2022-09-27T19:35:58Z

packages/website/src/components/ast/Elements.tsx

@@ -47,7 +47,7 @@ export function ComplexItem({
    if (selected && !isExpanded) {
      setIsExpanded(selected);
    }
-  }, [selection, data]);
+  }, [selection, data, level, isExpanded]);


Introduced bug somewhere around AST elements: you can no longer collapse an element that's visually focused.

See recording: the first 3/4 of time is in the deploy preview, where clicking a node in the source code to expand on the right, then trying to collapse, is not allowed.

Screen.Recording.2022-09-27.at.3.33.57.PM.mov

Thank you for pointing it out. It should be fixed on commit 507c7a5.

…ive-deps

amorimr · 2022-09-28T01:10:36Z

packages/website/src/components/editor/LoadedEditor.tsx

-  useEffect(
-    debounce(() => {
+  useEffect(() => {
+    const lintEditor = debounce(() => {


I've updated this hook because otherwise ESLint cannot parse correctly its dependencies and throws a warning. The functionality remains the same:

JoshuaKGoldberg

Swell, thanks! 🙌

armano2 · 2023-01-22T20:11:29Z

@JoshuaKGoldberg i missed this Pr, this change is not that great, there is alot of side effects and we are doing way to many updates now, eg, "constructor" and "destructor" is triggered way to often

I noticed also that even if there are no changes to config or code we are triggering linting eg, when user resizes window, changes theme etc.

JoshuaKGoldberg · 2023-01-23T13:12:44Z

🤔 @armano2 I'm not sold that we should completely revert this PR. #6335 is irksome and the excess linting is a little annoying, but I'm very hesitant to go against the rules of hooks. I think a larger issue with the playground code either way is that it sets up a lot of effects-driven logic in LoadedEditor & co. that IMO ideally should be more events-driven. What do you think?

armano2 · 2023-01-28T01:30:41Z

ideally should be more events-driven. What do you think?

I agree,

amorimr added 2 commits September 15, 2022 12:36

enable rules

6250d99

fix warnings

0c70dca

amorimr commented Sep 15, 2022

View reviewed changes

JoshuaKGoldberg reviewed Sep 15, 2022

View reviewed changes

Merge remote-tracking branch 'upstream/main' into enable-react-exaust…

1e4239a

…ive-deps

JoshuaKGoldberg requested changes Sep 27, 2022

View reviewed changes

JoshuaKGoldberg added the awaiting response Issues waiting for a reply from the OP or another party label Sep 27, 2022

amorimr added 4 commits September 27, 2022 16:28

Merge remote-tracking branch 'upstream/main' into enable-react-exaust…

9996cd3

…ive-deps

add missing dependencies

59b66ac

fix infinite rerenders

c025a31

fix expansion of tree items

507c7a5

amorimr commented Sep 28, 2022

View reviewed changes

JoshuaKGoldberg approved these changes Sep 28, 2022

View reviewed changes

JoshuaKGoldberg merged commit fab9974 into typescript-eslint:main Sep 28, 2022

amorimr deleted the enable-react-exaustive-deps branch September 28, 2022 14:57

github-actions bot locked as resolved and limited conversation to collaborators Oct 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore(website): Enable react-hooks exhaustive deps rules #5663

chore(website): Enable react-hooks exhaustive deps rules #5663

amorimr commented Sep 15, 2022

nx-cloud bot commented Sep 15, 2022 •

edited

Loading

typescript-eslint bot commented Sep 15, 2022

netlify bot commented Sep 15, 2022 •

edited

Loading

amorimr Sep 15, 2022

amorimr Sep 15, 2022

amorimr Sep 15, 2022

amorimr Sep 15, 2022

JoshuaKGoldberg left a comment

codecov bot commented Sep 25, 2022 •

edited

Loading

JoshuaKGoldberg left a comment

JoshuaKGoldberg Sep 27, 2022

amorimr Sep 28, 2022

amorimr Sep 28, 2022

JoshuaKGoldberg left a comment

armano2 commented Jan 22, 2023 •

edited

Loading

JoshuaKGoldberg commented Jan 23, 2023

armano2 commented Jan 28, 2023

chore(website): Enable react-hooks exhaustive deps rules #5663

chore(website): Enable react-hooks exhaustive deps rules #5663

Conversation

amorimr commented Sep 15, 2022

PR Checklist

Overview

nx-cloud bot commented Sep 15, 2022 • edited Loading

☁️ Nx Cloud Report

typescript-eslint bot commented Sep 15, 2022

netlify bot commented Sep 15, 2022 • edited Loading

✅ Deploy Preview for typescript-eslint ready!

amorimr Sep 15, 2022

Choose a reason for hiding this comment

amorimr Sep 15, 2022

Choose a reason for hiding this comment

amorimr Sep 15, 2022

Choose a reason for hiding this comment

amorimr Sep 15, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 25, 2022 • edited Loading

Codecov Report

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

JoshuaKGoldberg Sep 27, 2022

Choose a reason for hiding this comment

amorimr Sep 28, 2022

Choose a reason for hiding this comment

amorimr Sep 28, 2022

Choose a reason for hiding this comment

JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

armano2 commented Jan 22, 2023 • edited Loading

JoshuaKGoldberg commented Jan 23, 2023

armano2 commented Jan 28, 2023

nx-cloud bot commented Sep 15, 2022 •

edited

Loading

netlify bot commented Sep 15, 2022 •

edited

Loading

codecov bot commented Sep 25, 2022 •

edited

Loading

armano2 commented Jan 22, 2023 •

edited

Loading