1553931 - Add eslint-plugin-react-hooks to mozilla central

Status: RESOLVED FIXED

(Developer Infrastructure :: Lint and Formatting, task, P1)

Description

[Kate Hudson :k88hudson]

In Bug 1529007 we added eslint-plugin-react-hooks to the activity-stream repository's eslint config, which has some helpful rules around using react hooks. In order to support this with ./mach lint we would need to add this to the mozilla central package.json

Is this something that would make sense for us going forward?

Comment 1

[Dan Mosedale (:dmosedale, :dmose)]

We should consider doing this now that we're using hooks and effects in about:welcome.

Comment 2

[Meg Viar [:mviar]]

Using effects in conditionals came up on two different Messaging systems reviews recently. We should consider revisiting this.

Comment 3

[Punam Dahiya [:pdahiya]]

NI Mark for feedback on adding eslint for react hooks, thanks!

Comment 4

[Mark Banner (:standard8)]

I see no problem with eslint-plugin-react-hooks being included. The license is MIT which is acceptable, and its a small standalone module which is even better.

You'll need someone to do the work of getting the rules set up and passing. I would strongly recommend we enable them for the whole tree rather than just newtab. I don't know if that would affect some of the devtools code - they are the only other place that use react.

The approximate steps would be:

• ./mach npm install --save-exact --save-dev eslint-plugin-react-hooks
• Add the plugin reference and rules to enable to tools/lint/eslint/eslint-plugin-mozilla/lib/configs/recommended.js
• Run ./mach eslint * & fix the issues/tweat the rules as necessary.

If there are lots of issues, we can consider enabling them as warnings to begin with, but we should have a confirmed piece of work to get those down to zero.

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

The pocket component is using React as well these days: https://searchfox.org/mozilla-central/source/browser/components/pocket

Comment 6

[Ed Lee :Mardak]

(In reply to Mark Banner (:standard8) from comment #4)

I would strongly recommend we enable them for the whole tree rather than just newtab

Looks like the 1 default recommended "error" rule "react-hooks/rules-of-hooks" https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/eslint-plugin-react-hooks/src/index.js#L17

… checks any camelCase variables that start with "use" https://github.com/facebook/react/blob/cae635054e17a6f107a39d328649137b83f25972/packages/eslint-plugin-react-hooks/src/RulesOfHooks.js#L13-L18

This results in a bunch of false positive errors like…
TEST-UNEXPECTED-ERROR | toolkit/components/search/tests/xpcshell/test_region_params.js:30:9 | React Hook "SearchTestUtils.useTestEngines" is called in function "setup" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use". (react-hooks/rules-of-hooks)

I don't think we want to generally burden developers to figure out why they can't name their stuff with "use"

The parts of the codebase that does use react seem to pass fine (newtab, pocket, devtools).

Comment 7

[Mark Banner (:standard8)]

(In reply to Ed Lee :Mardak from comment #6)

(In reply to Mark Banner (:standard8) from comment #4)
This results in a bunch of false positive errors like…
TEST-UNEXPECTED-ERROR | toolkit/components/search/tests/xpcshell/test_region_params.js:30:9 | React Hook "SearchTestUtils.useTestEngines" is called in function "setup" that is neither a React function component nor a custom React Hook function. React component names must start with an uppercase letter. React Hook names must start with the word "use". (react-hooks/rules-of-hooks)

Urgh, I guess it is making the assumption that all files are React :( This might make selecting where to enable this difficult.

I don't think we want to generally burden developers to figure out why they can't name their stuff with "use"

Agreed.

Presumably not all our React code is using jsx files? If that's the case, we might have to selectively enable per various directories.

Comment 8

[Ed Lee :Mardak]

(In reply to Mark Banner (:standard8) from comment #7)

Presumably not all our React code is using jsx files? If that's the case, we might have to selectively enable per various directories.

Nod. Devtools uses plain .js https://searchfox.org/mozilla-central/search?q=import+React+from+%22react%22
Pocket has a mix of both .js and .jsx.

I'll conditionally extend from top level .eslintrc

Comment 9

[Ed Lee :Mardak]

Attachment: Bug 1553931 - Add eslint-plugin-react-hooks and enable for paths using React r?Standard8

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a43438413be
Add eslint-plugin-react-hooks and enable for paths using React r=Standard8

Comment 11

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/8a43438413be