Bring back eslint-plugin-react-hooks and fix issues #1589

swissspidy · 2020-05-06T13:53:35Z

Summary

Adds back missing eslint-plugin-react-hooks dependency and fixes all reported issues.

Relevant Technical Choices

Re-enables the react-hooks/rules-of-hooks rule.

To-do

Fix outstanding lint errors

Fixes #1562

Makes it clear it is a React component

No need for useCallback here

swissspidy · 2020-05-06T14:21:47Z

I thought the additionalHooks option in the ESLint config would eliminate most of the reported errors. Am I missing something? 🤔

carloskelly13 · 2020-05-06T14:40:24Z

It seems this plugin "assumes that any function starting with ”use” and a capital letter right after it is a Hook." so I think it should now catch everything.

swissspidy · 2020-05-06T14:55:24Z

Code in question: https://github.com/facebook/react/blob/7992ca10df497002e0e91bb5bfbc9661c9c85b88/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1492-L1533

I see that version 4.0.0 is brand new and had some breaking changes: https://github.com/facebook/react/blob/master/packages/eslint-plugin-react-hooks/CHANGELOG.md

Hmm, turns out they now treat any hook ending in Effect the same as useEffect (facebook/react#18580), but doesn't seem to be in 4.0.0.

Also, refs shouldn't need to be in the dependencies list.

dvoytenko · 2020-05-07T16:48:47Z

@swissspidy So it looks like the linter wants us to rearrange arguments in the keyboard hooks? Is this at all configurable? Stylistically, for these components, it's better to have two last arguments to be function + deps. But it looks like the linter always wants the first two?

swissspidy · 2020-05-07T17:22:59Z

Right 🤦 somehow I didn't see the forest for the trees here.

Yes, the linter expects the first two to be callback + deps. See https://github.com/facebook/react/blob/edf6eac8a181860fd8a2d076a43806f1237495a1/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L1492-L1533

This is currently not configurable, however they might be willing to allow this. They seem to be very open about contributions to the lint rule.

dvoytenko · 2020-05-07T21:39:16Z

This is currently not configurable, however they might be willing to allow this. They seem to be very open about contributions to the lint rule.

Ok. Let's try it. I sent them a pull request: facebook/react#18861. But we may have to be prepared to change signatures if it doesn't move fast enough. This is an important linter to avoid bugs.

swissspidy · 2020-05-20T13:40:44Z

assets/src/edit-story/components/colorPicker/editablePreview.js

@@ -60,6 +60,7 @@ function EditablePreview({ label, value, width, format, onChange }) {
  );

  // Handle ESC keypress to toggle input field.
+  //eslint-disable-next-line react-hooks/exhaustive-deps
  useKeyDownEffect(wrapperRef, { key: 'esc', editable: true }, disableEditing, [
    isEditing,


Adding disableEditing as the only dependency (as enforced by the rule) breaks the test in assets/src/edit-story/components/colorPicker/test/colorPicker/footerInteraction.js. The callback is simply not fired.

barklund

Looks good, just a couple of nits and comments.

barklund · 2020-05-20T14:24:48Z

assets/src/edit-story/components/colorPicker/useKeyDeleteStop.js

@@ -27,7 +27,7 @@ function useKeyDeleteStop(ref, onDelete) {
      ref.current.focus();
      onDelete();
    },
-    []
+    [onDelete, ref]


We shouldn't ever need to have refs as dependencies - as they're always stable. Can we somehow annotate that anything named *ref is in fact a ref?

Not sure, I'd have to check how the rule works under the hood.

We can skip this for now, but we do have a lot of those around - would be awesome if we could however set the linter up to assume, that anything named /[Rr]ef$/ and only accessed via .current is a stable ref and never needs to be in dependencies. It might be wishful thinking though.

+1 to not have refs as deps. but in this case it arrives from outside so there's a limited info that this is indeed a ref.

assets/src/edit-story/components/inspector/document/publish/index.js

assets/src/edit-story/components/colorPicker/editablePreview.js

assets/src/edit-story/components/library/libraryTabs.js

assets/src/edit-story/components/reorderable/reorderable.js

assets/src/edit-story/utils/useLiveRegion.js

barklund

Looks good to me!

assets/src/edit-story/components/panels/alignment/alignment.js

.eslintrc

assets/src/edit-story/components/panels/alignment/alignment.js

patches/eslint-plugin-react-hooks+4.0.0.patch

spacedmonkey · 2020-05-20T16:07:31Z

Small nitpicks, but we are looking good.

dvoytenko · 2020-05-21T18:12:39Z

@swissspidy sorry for delay. I just rebased the original proposal pull request. patch-package for this is a brilliant!

assets/src/edit-story/utils/context.js

dvoytenko · 2020-05-21T18:28:23Z

patches/eslint-plugin-react-hooks+4.0.0.patch

@@ -0,0 +1,52 @@
+diff --git a/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js b/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js


@swissspidy is there no way at all here to actually put the pull request reference? Or even maybe automatically pull the diff from the pull request? I'm asking since I just rebased the pull request and I'll have to do it probably again and again for some time until it's accepted, hopefully.

I don't think there's any automated way for this, but I'll have a look.

* master: (101 commits) Update list of Google Fonts Fix google fonts build script Use data arg for force=true instead of as a literal string Added call to delete API for media in apiProvider Fix using incorrect input for adding color preset. (#1933) only use shapes and background colors for dummy story data to avoid local host image issues Bump percy from 0.26.6 to 0.26.7 (#1937) Renamed animation components for better clarity Add custom calendar implementation (#1743) Fixed lint errors Added tests to AMPKeyframes Bump uuid from 8.0.0 to 8.1.0 (#1909) Bump @wordpress/icons from 1.4.0 to 2.0.0 (#1928) Bring back eslint-plugin-react-hooks and fix issues (#1589) Use use-context-selector versions of createContext, useContext Add useContextSelectorShallow hook Moved SimpleAnimation to root of parts Dashboard: Converted BlinkOn animation to new standard splitting out storiesView and emptyView from content. Adding tests in content/test and storybook examples in content/ . Force stylelint to 13.3.3, because 13.5.0 was failing ...

swissspidy added 4 commits May 6, 2020 15:39

Add back missing eslint-plugin-react-hooks dependency

5dbd036

Re-enable react-hooks/rules-of-hooks

7cb61c6

Use PascalCase for PopoverCard component

5466b56

Makes it clear it is a React component

Remove condition before useEffect hook in AutoSaveHandler

e923cac

swissspidy added the Type: Infrastructure Changes impacting testing infrastructure or build tooling label May 6, 2020

googlebot added the cla: yes label May 6, 2020

swissspidy changed the title ~~Fix/eslint plugin react hooks~~ Bring back eslint-plugin-react-hooks and fix issues May 6, 2020

swissspidy mentioned this pull request May 6, 2020

Add eslint-plugin-react-hooks to the JavaScript project #1562

Closed

This comment has been minimized.

Sign in to view

swissspidy added 3 commits May 6, 2020 16:05

MediaProvider: use function directly for useEffect hook

5c26291

No need for useCallback here

useLiveRegion: no need for useCallback

60a43d3

useUploadVideoFrame: add missing dependencies

d7038c4

swissspidy requested review from barklund and carloskelly13 May 6, 2020 14:26

Merge branch 'master' into fix/eslint-plugin-react-hooks

b0edb94

swissspidy added the Help wanted label May 6, 2020

swissspidy requested a review from dvoytenko May 6, 2020 16:31

swissspidy added 5 commits May 7, 2020 11:58

Merge branch 'master' into fix/eslint-plugin-react-hooks

30034ef

Merge branch 'master' into fix/eslint-plugin-react-hooks

c48e49c

Fix rules of hooks error in Reorderable component

f53b380

Add dep to useEffect

24f5dd3

Merge branch 'master' into fix/eslint-plugin-react-hooks

e4586f5

dvoytenko approved these changes May 7, 2020

View reviewed changes

swissspidy requested review from BrittanyIRL, mariano-formidable, pbakaus and spacedmonkey as code owners May 20, 2020 13:11

swissspidy removed request for pbakaus, BrittanyIRL and mariano-formidable May 20, 2020 13:11

swissspidy removed Status: Blocked On hold for the time being AMP Format Issues that need to be addressed in the AMP story format labels May 20, 2020

Suppress warning to fix test

ccc8fed

swissspidy commented May 20, 2020

View reviewed changes

barklund reviewed May 20, 2020

View reviewed changes

swissspidy added 3 commits May 20, 2020 16:38

Add back useCallback usage

c5f3a9a

Simplify callbacks

b354058

lint fixes

4beaf6f

barklund approved these changes May 20, 2020

View reviewed changes

assets/src/edit-story/components/panels/alignment/alignment.js Outdated Show resolved Hide resolved

spacedmonkey reviewed May 20, 2020

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

spacedmonkey reviewed May 20, 2020

View reviewed changes

assets/src/edit-story/components/panels/alignment/alignment.js Show resolved Hide resolved

spacedmonkey reviewed May 20, 2020

View reviewed changes

patches/eslint-plugin-react-hooks+4.0.0.patch Show resolved Hide resolved

swissspidy added 4 commits May 21, 2020 09:23

Simplify callbacks

0597d95

Do not comment out disabled eslint setting

3f0d976

Merge branch 'master' into fix/eslint-plugin-react-hooks

b48bbff

Prevent conditional hook calling in context.js

4e2d61a

swissspidy merged commit 3073f27 into master May 21, 2020

swissspidy deleted the fix/eslint-plugin-react-hooks branch May 21, 2020 07:42

dvoytenko reviewed May 21, 2020

View reviewed changes

assets/src/edit-story/utils/context.js Show resolved Hide resolved

dvoytenko reviewed May 21, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bring back eslint-plugin-react-hooks and fix issues #1589

Bring back eslint-plugin-react-hooks and fix issues #1589

swissspidy commented May 6, 2020 •

edited

Loading

This comment has been minimized.

This comment has been minimized.

swissspidy commented May 6, 2020

carloskelly13 commented May 6, 2020

swissspidy commented May 6, 2020

dvoytenko commented May 7, 2020

swissspidy commented May 7, 2020

dvoytenko commented May 7, 2020

swissspidy May 20, 2020

barklund left a comment

barklund May 20, 2020

swissspidy May 20, 2020

barklund May 20, 2020

dvoytenko May 21, 2020

barklund left a comment

spacedmonkey commented May 20, 2020

dvoytenko commented May 21, 2020

dvoytenko May 21, 2020

swissspidy May 25, 2020

		@@ -0,0 +1,52 @@
		diff --git a/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js b/node_modules/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js

Bring back eslint-plugin-react-hooks and fix issues #1589

Bring back eslint-plugin-react-hooks and fix issues #1589

Conversation

swissspidy commented May 6, 2020 • edited Loading

Summary

Relevant Technical Choices

To-do

This comment has been minimized.

This comment has been minimized.

swissspidy commented May 6, 2020

carloskelly13 commented May 6, 2020

swissspidy commented May 6, 2020

dvoytenko commented May 7, 2020

swissspidy commented May 7, 2020

dvoytenko commented May 7, 2020

swissspidy May 20, 2020

Choose a reason for hiding this comment

barklund left a comment

Choose a reason for hiding this comment

barklund May 20, 2020

Choose a reason for hiding this comment

swissspidy May 20, 2020

Choose a reason for hiding this comment

barklund May 20, 2020

Choose a reason for hiding this comment

dvoytenko May 21, 2020

Choose a reason for hiding this comment

barklund left a comment

Choose a reason for hiding this comment

spacedmonkey commented May 20, 2020

dvoytenko commented May 21, 2020

dvoytenko May 21, 2020

Choose a reason for hiding this comment

swissspidy May 25, 2020

Choose a reason for hiding this comment

swissspidy commented May 6, 2020 •

edited

Loading