feat(eslint-plugin-react-hooks): convert to typescript and package type declarations #32240

michaelfaith · 2025-01-28T02:02:04Z

Summary

This change converts the eslint hooks plugin to typescript, which also allows us to include type declarations in the package, for those using typescript eslint configs.

Constituent changes that should land before this one

fix(eslint-plugin-react-hooks): remove experimental checks involving useEffectEvent #32276
build(eslint-plugin-react-hooks): add dev dependencies for typescript migration #32279
add tsconfig to the package along with the new estree and global type declarations (PR pending)
updates to the existing build infra to support typescript packages (PR pending)

Closes #30119

I isolated this mv into its own PR to ensure we're able to maintain commit history for these files.

…pe declarations This change converts the eslint hooks plugin to typescript, which also allows us to include type declarations in the package, for those using [typescript eslint configs](https://eslint.org/blog/2025/01/eslint-v9.18.0-released/#stable-typescript-configuration-file-support).

michaelfaith · 2025-01-28T02:04:10Z

Core migration is done. Tests are green when run locally. Now working on integrating it into the existing build infra and developing e2e test fixtures.

poteto

Thanks for working on this! I think as it is the PR is bordering on being way too large to review thoroughly. It'd be good if we can split this up into a few smaller PRs. For example the peripheral changes (installing/updating packages) or script/babel/build changes could also be split.

I would also recommend not rewriting existing patterns like for loops to for..of and x == null to !x, since they have subtle differences. Keep the conversion as 1:1 as possible unless it's strictly necessary for type soundness. Additionally if you need to add new code (eg adding continue inside of a loop), could you add more context on why it's necessary?

poteto · 2025-01-28T15:39:45Z

packages/eslint-plugin-react-hooks/src/index.ts

+import ExhaustiveDeps from './ExhaustiveDeps';
+import type {ESLint, Linter, Rule} from 'eslint';
+
+const packageJson: {name: string; version: string} = require('../package.json');


Requiring package.json doesn't play nice with Meta's internal stuff so I would prefer hardcoding or dropping this. I'm also not sure there's much value in adding the name and version to the meta tag, what's the use case?

One use case is caching. ESLint uses the meta field of a plugin to know when it's safe to hit cache for its output. Since this is a relatively expensive plugin (especially when the compiler plugin is folded in) it would be nice to keep the field if possible.

(Depending on how Meta consumes this package, maybe package.json can be read at build time and the values can be replaced/inlined?)

(Depending on how Meta consumes this package, maybe package.json can be read at build time and the values can be replaced/inlined?)

Yeah this would be preferrable, although I can see that as a follow up PR while this PR either omits or hardcodes those values for now.

I'll make that update, and add a TODO to do the CI version as a follow-up.

poteto · 2025-01-28T15:40:32Z

packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js

+global.__EXPERIMENTAL__ =
+  global.__EXPERIMENTAL__ ?? process.env.__EXPERIMENTAL__;
+


Is this still needed with the global type declaration?

This is just for the unit tests. By reading the value from an environment variable when it's not been provided by CI, I can quickly run unit tests just for this package locally by setting or unsetting that env variable. It's just for helping to improve the local dev loop / reduce the feedback loop when making changes. I can stash those locally if you prefer, but I figured it would benefit anyone who works on the plugin after me?

It looks like it's only used for the useEffectEvent lint which we want to change eventually anyway. I think we can just remove useEffectEvent in the lint rule(s) for now (separate PR), and then you won't need to set __EXPERIMENTAL__ at all. Not that it matters after removal but for reference we also set that value through a different env variable.

Sounds good. I can prepare a separate change for that. It'd probably make sense to put it in before this PR, right?

poteto · 2025-01-28T15:41:08Z

packages/eslint-plugin-react-hooks/babel.config.js

do we need babel and esbuild?

This is just to satisfy running jest at the package level, helping to improve the local dev loop. Similar to the other change, I can stash it locally if you prefer, but just wanted to make it dead simple to run tests locally.

Gotcha! Seems fine to keep around. Could you add a comment stating that it's for tests only and not used as part of the build?

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.ts

poteto · 2025-01-28T15:46:17Z

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.ts

@@ -278,26 +330,23 @@ export default {
              if (name === 'useState') {
                const references = resolved.references;
                let writeCount = 0;
-                for (let i = 0; i < references.length; i++) {
-                  if (references[i].isWrite()) {
+                for (const reference of references) {


Let's keep the previous for loop, it tends to output tighter loops with less overhead after builds

I changed many of the for loops to be for of due to the strictness of the tsconfig (I used the same settings as what was in the compiler plugin tsconfig). Using strict in the tsconfig, it's unforgiving with undefinedness. And with the traditional for loop, TS can't be certain that the array reference is in range, so it would require a lot of non-null assertions (!) to satisfy that level of strictness, and those aren't great to have all over the place and would be considered a smell. With for of loops, there isn't the same issue with values being possibly undefined. Would you rather I go with the non-null assertions or relax the strictness of the tsconfig?

Oh! And I also saw there were already a few for of loops, and someone had previously disabled the eslint rule for the file, so I figured that was the best option.

Ah I see, that makes sense. Probably fine to convert everything to for..of in that case

poteto · 2025-01-28T15:51:23Z

packages/eslint-plugin-react-hooks/src/index.ts

+    'recommended-latest': {
+      name: 'react-hooks/recommended',
+      plugins: {
+        get 'react-hooks'(): ESLint.Plugin {


does this need to be a getter?

Since it's self-referential, returning the instance of the plugin that's being instantiated, it needs to be a function here to have proper types applied. If it were a property that returned plugin, typescript doesn't have enough information about the final shape of the type at that point. So, by making it a function / getter, we can tell TypeScript what type to expect here, with the getter's return type. The alternative would be set the plugins property after the plugin object is created, but then we'd lose time information for that property on the plugin / configs.

Avoids this:

poteto · 2025-01-28T16:05:35Z

packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.ts

+          if (!referenceNode) {
+            continue;
+          }


Why is this needed?

fastFindReferenceWithParent returns null if it's unable to find a suitable node, but all the logic below this assumes that it's returned a Node. In practice, it shouldn't really ever not return a node, so this is more of an edge case, but since the function isn't narrowing out null, this early return keeps the rest of the assumptions in tact.

…pe declarations This change converts the eslint hooks plugin to typescript, which also allows us to include type declarations in the package, for those using [typescript eslint configs](https://eslint.org/blog/2025/01/eslint-v9.18.0-released/#stable-typescript-configuration-file-support).

michaelfaith · 2025-01-29T23:19:24Z

@poteto thanks for taking a look at this already! I've made some updates and responded to several of the feedback items.

It'd be good if we can split this up into a few smaller PRs. For example the peripheral changes (installing/updating packages) or script/babel/build changes could also be split.

I'm thinking these could be good candidates for breaking out as separate changes, but wanted to get your input:

removing the useEffectEvent / EXPERIMENTAL checks you mentioned
Add dependencies
Add tsconfig and the two d.ts files in the types directory

Thoughts?

poteto

Thanks for addressing the previous feedback!

poteto · 2025-01-30T17:10:15Z

packages/eslint-plugin-react-hooks/src/RulesOfHooks.ts


      // Process our code path.
      //
      // Everything is ok if all React Hooks are both reachable from the initial
      // segment and reachable from every final segment.
      onCodePathEnd(codePath, codePathNode) {
        const reactHooksMap = codePathReactHooksMapStack.pop();
-        if (reactHooksMap.size === 0) {
+        if (!reactHooksMap?.size) {


Could we keep the previous comparison to 0?

Suggested change

if (!reactHooksMap?.size) {

if (reactHooksMap?.size === 0) {

poteto · 2025-01-30T17:12:17Z

packages/eslint-plugin-react-hooks/src/RulesOfHooks.ts

-          for (const ref of reference.resolved.references) {
+          for (const ref of reference.resolved?.references || []) {


For these types of changes, rather than relax the code by handling undefined behavior, I think we should prefer to throw/invariant instead if the constraint isn't being met. That helps us find any issues with our assumptions earlier instead of hiding it.

poteto · 2025-01-30T17:16:27Z

@poteto thanks for taking a look at this already! I've made some updates and responded to several of the feedback items.

It'd be good if we can split this up into a few smaller PRs. For example the peripheral changes (installing/updating packages) or script/babel/build changes could also be split.

I'm thinking these could be good candidates for breaking out as separate changes, but wanted to get your input:

removing the useEffectEvent / EXPERIMENTAL checks you mentioned

Add dependencies

Add tsconfig and the two d.ts files in the types directory

Thoughts?

That split makes sense. I would also try to get the build changes done in an earlier PR, and this PR could be for just the changes to the rules themselves.

michaelfaith · 2025-01-30T18:44:56Z

I would also try to get the build changes done in an earlier PR, and this PR could be for just the changes to the rules themselves.

Sounds good. I updated the description of this PR to include tasks for each of those, and will link PRs as I work through each.

…useEffectEvent Contributing to facebook#32240, this change removes the experimental `useEffectEvent` checks in the two react hooks eslint rule. As the api is refined, and the compiler rule(s) are incorporated into this plugin, these checks will be covered then.

… migration Contributing to facebook#32240, this change adds the dev dependencies needed to support the migration of the plugin to typescript.

michaelfaith added 2 commits January 20, 2025 13:17

refactor(eslint-plugin-react-hooks): rename js files to ts

804f4b4

I isolated this mv into its own PR to ensure we're able to maintain commit history for these files.

facebook-github-bot added the CLA Signed label Jan 28, 2025

vercel bot deployed to Preview January 28, 2025 02:05 View deployment

poteto reviewed Jan 28, 2025

View reviewed changes

This comment was marked as off-topic.

Sign in to view

vercel bot deployed to Preview January 29, 2025 23:14 View deployment

poteto reviewed Jan 30, 2025

View reviewed changes

michaelfaith mentioned this pull request Jan 31, 2025

fix(eslint-plugin-react-hooks): remove experimental checks involving useEffectEvent #32276

Open

michaelfaith mentioned this pull request Jan 31, 2025

build(eslint-plugin-react-hooks): add dev dependencies for typescript migration #32279

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(eslint-plugin-react-hooks): convert to typescript and package type declarations #32240

feat(eslint-plugin-react-hooks): convert to typescript and package type declarations #32240

michaelfaith commented Jan 28, 2025 •

edited

Loading

michaelfaith commented Jan 28, 2025 •

edited

Loading

poteto left a comment

poteto Jan 28, 2025

billyjanitsch Jan 28, 2025

billyjanitsch Jan 28, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025

michaelfaith Jan 29, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025

poteto Jan 29, 2025 •

edited

Loading

michaelfaith Jan 29, 2025

poteto Jan 30, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025

poteto Jan 29, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025 •

edited

Loading

michaelfaith Jan 29, 2025 •

edited

Loading

poteto Jan 30, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025

poteto Jan 28, 2025

michaelfaith Jan 29, 2025 •

edited

Loading

This comment was marked as off-topic.

michaelfaith commented Jan 29, 2025

poteto left a comment

poteto Jan 30, 2025

poteto Jan 30, 2025

poteto commented Jan 30, 2025

michaelfaith commented Jan 30, 2025 •

edited

Loading

		global.__EXPERIMENTAL__ =
		global.__EXPERIMENTAL__ ?? process.env.__EXPERIMENTAL__;

		for (const ref of reference.resolved.references) {
		for (const ref of reference.resolved?.references \|\| []) {

feat(eslint-plugin-react-hooks): convert to typescript and package type declarations #32240

Are you sure you want to change the base?

feat(eslint-plugin-react-hooks): convert to typescript and package type declarations #32240

Conversation

michaelfaith commented Jan 28, 2025 • edited Loading

Summary

Constituent changes that should land before this one

michaelfaith commented Jan 28, 2025 • edited Loading

poteto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poteto Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfaith Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

michaelfaith Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michaelfaith Jan 29, 2025 • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

michaelfaith commented Jan 29, 2025

poteto left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

poteto commented Jan 30, 2025

michaelfaith commented Jan 30, 2025 • edited Loading

michaelfaith commented Jan 28, 2025 •

edited

Loading

michaelfaith commented Jan 28, 2025 •

edited

Loading

poteto Jan 29, 2025 •

edited

Loading

michaelfaith Jan 29, 2025 •

edited

Loading

michaelfaith Jan 29, 2025 •

edited

Loading

michaelfaith Jan 29, 2025 •

edited

Loading

michaelfaith commented Jan 30, 2025 •

edited

Loading