Summary:
I noticed there's a bug where the lint will recognize the type on a cast
annotation as a missing dependency;
```
function MyComponent() {
type ColumnKey = 'id' | 'name';
type Item = {id: string, name: string};
const columns = useMemo(
() => [
{
type: 'text',
key: 'id',
} as TextColumn<ColumnKey, Item>,
^^^^^^^^ here
],
[],
);
}
```
This is due to the AST of AsExpressions being something like:
AsExpression
└── typeAnnotation: GenericTypeAnnotation
└── typeParameters: TypeParameterInstantiation
└── params[0]: GenericTypeAnnotation
└── id: Identifier (name: "ColumnKey")
Where `ColumnKey` never has a TypeParameter Annotation. So we need to
consider it to be a flow type due to it belonging to a
GenericTypeAnnotation
Test Plan:
Added unit tests
Before:
```
Test Suites: 1 failed, 2 passed, 3 total
Tests: 2 failed, 5065 passed, 5067 total
Snapshots: 0 total
Time: 16.517 s
Ran all test suites.
error Command failed with exit code 1.
```
After:
```
PASS __tests__/ReactCompilerRuleTypescript-test.ts
PASS __tests__/ESLintRulesOfHooks-test.js (6.192 s)
PASS __tests__/ESLintRuleExhaustiveDeps-test.js (9.97 s)
Test Suites: 3 passed, 3 total
Tests: 5067 passed, 5067 total
Snapshots: 0 total
Time: 10.21 s, estimated 11 s
Ran all test suites.
✨ Done in 12.66s.
```
Add a fast heuristic to detect whether a file may contain React
components or hooks before running the full compiler. This avoids the
overhead of Babel AST parsing and compilation for utility files, config
files, and other non-React code.
The heuristic uses ESLint's already-parsed AST to check for functions
with React-like names at module scope:
- Capitalized functions: MyComponent, Button, App
- Hook pattern functions: useEffect, useState, useMyCustomHook
Files without matching function names are skipped and return an empty
result, which is cached to avoid re-checking for subsequent rules.
Also adds test coverage for the heuristic edge cases.
`react-hooks/exhaustive-effect-dependencies` from
`ValidateExhaustiveDeps` reports errors for both missing and extra
effect deps. We already have `react-hooks/exhaustive-deps` that errors
on missing dependencies. In the future we'd like to consolidate this all
to the compiler based error, but for now there's a lot of overlap. Let's
enable testing the extra dep warning by splitting out reporting modes.
This PR
- Creates `on`, `off`, `missing-only`, and `extra-only` reporting modes
for the effect dep validation flag
- Temporarily enables the new rule with `extra-only` in
`eslint-plugin-react-hooks`
- Adds additional null checking to `manualMemoLoc` to fix a bug found
when running against the fixture
This deprecates the `noEmit: boolean` flag and adds `outputMode:
'client' | 'client-no-memo' | 'ssr' | 'lint'` as the replacement.
OutputMode defaults to null and takes precedence if specified, otherwise
we use 'client' mode for noEmit=false and 'lint' mode for noEmit=true.
Key points:
* Retrying failed compilation switches from 'client' mode to
'client-no-memo'
* Validations are enabled behind
Environment.proto.shouldEnableValidations, enabled for all modes except
'client-no-memo'. Similar for dropping manual memoization.
* OptimizeSSR is now gated by the outputMode==='ssr', not a feature flag
* Creation of reactive scopes, and related codegen logic, is now gated
by outputMode==='client'
We were not recording uEE calls in component/hook syntax. Easy fix.
Added tests matching function component syntax for component syntax +
added one for hooks
## Summary
Fixes https://github.com/facebook/react/issues/34793.
We are allowing passing down effect events when they are inlined as a
prop.
```
<Child onClick={useEffectEvent(...)} />
```
This seems like a case that someone not familiar with `useEffectEvent`'s
purpose could fall for so this PR introduces logic to disallow its
usage.
An alternative implementation would be to modify the name and function
of `recordAllUseEffectEventFunctions` to record all `useEffectEvent`
instances either assigned to a variable or not, but this seems clearer.
Or we could also specifically disallow its usage inside JSX. Feel free
to suggest any improvements.
## How did you test this change?
- Added a new test in
`packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js`.
All tests pass.
Renames the `recommended` property on LintRule to `preset`, to allow
exporting rules for different presets. For now the `Recommended` and
`RecommendedLatest` presets are the same, but in the next PR I will
enable more rules for the latest preset.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34782).
* #34783
* __->__ #34782
For 7.0.0:
Slim down presets to just 2 configurations:
- `recommended`: legacy and flat config with all recommended rules, and
- `recommended-latest`: legacy and flat config with all recommended
rules plus new bleeding edge experimental compiler rules
Removed:
- `recommended-latest-legacy`
- `flat/recommended`
Please see the README for new install instructions.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34757).
* #34783
* #34782
* __->__ #34757
Adds back HermesParser to eslint-plugin-react-hooks. There are still
[external users of
Flow](https://github.com/facebook/react/pull/34719#issuecomment-3368137743)
using the plugin, so we shouldn't break the plugin for them. However, we
still have the problem of double parsing: once from eslint (which we
discard) and then another via babel/hermes parser.
In the long run we should investigate a translation layer from estree to
babel (or alternatively, update the compiler to take estree as input).
But for now, I am reverting the PR.
This does mean that [Sandpack in
react.dev](11cb6b5915/src/components/MDX/Sandpack/runESLint.tsx (L31))
cannot update to the latest eprh as HermesParser does not appear to be
able to be run in a browser. I discovered this while trying to update
eprh on react.dev last week, but didn't investigate deeply. I'll need to
double check that again to find out more.
Another attempt to fix#34745. I updated our fixture for eslint-v9 to
include running tsc. I believe there were 2 issues:
1. `export * from './cjs/eslint-plugin-react-hooks'` in npm/index.d.ts
was no longer correct as we updated index.ts to export default instead
of named exports
2. After fixing ^ there was a typescript error which I fixed by making
some small tweaks
We will be focusing eslint-plugin-react-hooks as the primary OSS-only
package for our lint plugin. eslint-plugin-react-compiler will remain as
a Meta only package as some limitations of our internal infra require us
to use packages that aren't widely adopted by the rest of the industry.
This PR removes `hermes-parser`, which was meant to support parsing Flow
syntax.
This rule was a leftover from a while ago and doesn't actually lint
anything useful. Specifically, you get a lint error if you try to opt
out a component that isn't already bailing out. If there's a bailout the
compiler already safely skips over it, so adding `'use no memo'` there
is unnecessary.
Fixes#31407
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34703).
* __->__ #34703
* #34700
Previously, the `recommended` config used the legacy ESLint format
(plugins as an array of strings). This causes errors when used with
ESLint v9's `defineConfig()` helper. This was following [eslint's own
docs](https://eslint.org/docs/latest/extend/plugins#backwards-compatibility-for-legacy-configs):
> With this approach, both configuration systems recognize
"recommended". The old config system uses the recommended key while the
current config system uses the flat/recommended key. The defineConfig()
helper first looks at the recommended key, and if that is not in the
correct format, it looks for the flat/recommended key. This allows you
an upgrade path if you’d later like to rename flat/recommended to
recommended when you no longer need to support the old config system.
However,
[`isLegacyConfig()`](https://github.com/eslint/rewrite/blob/main/packages/config-helpers/src/define-config.js#L73-L81)
(also see
[`eslintrcKeys`](https://github.com/eslint/rewrite/blob/main/packages/config-helpers/src/define-config.js#L24-L35))
function doesn't check for the `plugins` key, so our config was
incorrectly treated as flat config despite being in legacy format.
This PR fixes the issue, along with a few other fixes combined:
1. Convert `recommended` to flat config format
2. Separate basic rules (exhaustive-deps, rules-of-hooks) from compiler
rules
3. Add `recommended-latest-legacy` config for non-flat config users who
want all recommended rules (including compiler rules)
4. Adding more types for the exported config
Our shipped presets in 6.x.x will essentially be:
- `recommended-legacy`: legacy (non-flat), with basic rules only
- `recommended-latest-legacy`: legacy (non-flat), all rules (basic +
compiler)
- `flat/recommended`: flat, basic rules only (now the same as
recommended, but to avoid making a breaking change we'll just keep it
around in 6.x.x)
- `recommended-latest`: flat, all rules (basic + compiler)
- `recommended`: flat, basic rules only
In the next breaking release 7.x.x, we will collapse down the presets
into three:
- `recommended-legacy`: all recommended rules
- `recommended`: all recommended rules
- `recommended-experimental`: all recommended rules + new bleeding edge
experimental rules
Closes#34679
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34700).
* #34703
* __->__ #34700
Follow up to #34649. This adds the compiler rules back so they can be
opted-in 6.1.0, but aren't included in the presets as that would be a
breaking change.
Called Before:
> `logEvent` is a function created with React Hook "useEffectEvent", and
can only be called from the same component.
Called After:
> `logEvent` is a function created with React Hook "useEffectEvent", and
can only be called from Effects and Effect Events in the same component.
Referenced Before:
> `logEvent` is a function created with React Hook "useEffectEvent", and
can only be called from the same component. They cannot be assigned to
variables or passed down.
Referenced After:
> `logEvent` is a function created with React Hook "useEffectEvent", and
can only be called from Effects and Effect Events in the same component.
It cannot be assigned to a variable or passed down.
Like in the diff below, we can read from the shared configuration to
check exhaustive deps.
I allow the classic additionalHooks configuration to override it so that
this change
is backwards compatible.
--
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34637).
* __->__ #34637
* #34497
We need to be able to specify additional effect hooks for the
RulesOfHooks lint rule
in order to allow useEffectEvent to be called by custom effects.
ExhaustiveDeps
does this with a regex suppplied to the rule, but that regex is not
accessible from
other rules.
This diff introduces a `react-hooks` entry you can put in the eslint
settings that
allows you to specify custom effect hooks and share them across all
rules.
This works like:
```
{
settings: {
'react-hooks': {
additionalEffectHooks: string,
},
},
}
```
The next diff allows useEffect to read from the same configuration.
----
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34497).
* #34637
* __->__ #34497
With #34176 we now have granular lint rules created for each compiler
ErrorCategory. However, we had remnants of our old error severities
still in use which makes reporting errors quite clunky. Previously you
would need to specify both a category and severity which often ended up
being the same.
This PR moves severity definition into our rules which are generated
from our categories. For now I decided to defer "upgrading" categories
from a simple string to a sum type since we are only using severities to
map errors to eslint severity.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34401).
* #34409
* #34404
* #34403
* #34402
* __->__ #34401
NOTE: this is a merged version of @mofeiZ's original PR along with my
edits per offline discussion. The description is updated to reflect the
latest approach.
The key problem we're trying to solve with this PR is to allow
developers more control over the compiler's various validations. The
idea is to have a number of rules targeting a specific category of
issues, such as enforcing immutability of props/state/etc or disallowing
access to refs during render. We don't want to have to run the compiler
again for every single rule, though, so @mofeiZ added an LRU cache that
caches the full compilation output of N most recent files. The first
rule to run on a given file will cause it to get cached, and then
subsequent rules can pull from the cache, with each rule filtering down
to its specific category of errors.
For the categories, I went through and assigned a category roughly 1:1
to existing validations, and then used my judgement on some places that
felt distinct enough to warrant a separate error. Every error in the
compiler now has to supply both a severity (for legacy reasons) and a
category (for ESLint). Each category corresponds 1:1 to a ESLint rule
definition, so that the set of rules is automatically populated based on
the defined categories.
Categories include a flag for whether they should be in the recommended
set or not.
Note that as with the original version of this PR, only
eslint-plugin-react-compiler is changed. We still have to update the
main lint rule.
## Test Plan
* Created a sample project using ESLint v9 and verified that the plugin
can be configured correctly and detects errors
* Edited `fixtures/eslint-v9` and introduced errors, verified that the w
latest config changes in that fixture it correctly detects the errors
* In the sample project, confirmed that the LRU caching is correctly
caching compiler output, ie compiling files just once.
Co-authored-by: Mofei Zhang <feifei0@meta.com>
Work in progress, i'm experimenting with revamping our diagnostic infra.
Starting with a better format for representing errors, with an ability
to point ot multiple locations, along with better printing of errors. Of
course, Babel still controls the printing in the majority case so this
still needs more work.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33751).
* #33981
* #33777
* #33767
* #33765
* #33760
* #33759
* #33758
* __->__ #33751
* #33752
* #33753
Summary:
useEffectEvent is meant to be used specifically in combination with
useEffect, and using
the feature in arbitrary closures can lead to surprising reactivity
semantics. In order to
minimize risk in the experimental rollout, we are going to restrict its
usage to being
called directly inside an effect or another useEffectEvent, effectively
enforcing the function
coloring statically. Without an effect system this is the best we can
do.
Summary:
To prepare for automatic effect dependencies, some codebases may want to
codemod
existing useEffect calls with no deps to include an explicit undefined
second argument
in order to preserve the "run on every render" behavior. In sufficiently
large codebases,
this may require a temporary enforcement period where all effects
provide an explicit
dependencies argument.
Outside of migration, relying on a component to render can lead to real
bugs,
especially when working with memoization.
This rule currently has a few false positives, so let's disable it for
now (just in the eslint rule, it's still enabled in the compiler) while
we iterate on it.
This change merges the `react-compiler` rule from
`eslint-plugin-react-compiler` into the `eslint-plugin-react-hooks`
plugin. In order to do the move in a way that keeps commit history with
the moved files, but also no remove them from their origin until a
future cleanup change can be done, I did the `git mv` first, and then
recreated the files that were moved in their original places, as a
separate commit. Unfortunately GH shows the moved files as new instead
of the ones that are truly new. But in the IDE and `git blame`, commit
history is intact with the moved files.
Since this change adds new dependencies, and one of those dependencies
has a higher `engines` declaration for `node` than what the plugin
currently has, this is technically a breaking change and will have to go
out as part of a major release.
### Related Changes
- https://github.com/facebook/react/pull/32458
---------
Co-authored-by: Lauren Tan <poteto@users.noreply.github.com>
This change swaps which config `recommended` is aliasing. In 5.2.0, the
new flat config was introduced as `recommended-latest`, while
`recommended` still pointed at the legacy rc-based config, with a note
that in the next major version `recommended` would be updated to point
at `recommend-latest`. This change makes that swap, and make the default
`recommended` experience the flat config. To continue using the legacy
rc recommended config, please make the following change in your config
```diff
- extends: ['plugin:react-hooks/recommended']
+ extends: ['plugin:react-hooks/recommended-legacy']
```
This change also deprecates `recommended-latest` in favor of
`recommended`. `recommended-latest` will be removed in a future major
version.
The README has been updated to reflect the new usage, and to put the
flat config sections before the legacy config sections.
I also took the opportunity to change the v9 fixture to use a typescript
config, serving as a demonstration for usage as well as a way to
validate the types are correct.
BREAKING CHANGE
---------
Co-authored-by: lauren <poteto@users.noreply.github.com>
Since the compiler plugin is going to be merged into the hooks plugin,
and ultimately decomposed into several more rules, it would be good to
start creating a more traditional folder structure for the plugin. This
change just moves the rules into a `rules` folder.
Co-authored-by: lauren <poteto@users.noreply.github.com>
- [build(eslint-plugin-react-hooks): add
ts-linting](4c0fbe73d9)
This change adds configuration to the eslint config governing
`eslint-plugin-react-hooks` to use the typescript-eslint plugin and
parser. It adds the typescript-recommended config, and configures the
team's preferred `array-type` convention.
- [refactor(eslint-plugin-react-hooks): improve
conditionals](540d0d95bc)
This change addresses several feedback items from
https://github.com/facebook/react/pull/32240
- [ci (eslint-e2e): exclude nested node_modules from
cache](a3279f46a8)
This change removes the nested fixture `node_modules` from being cached,
so that the symbolic link can be made after the build happens.
<!--
Thanks for submitting a pull request!
We appreciate you spending the time to work on these changes. Please
provide enough information so that others can review your pull request.
The three fields below are mandatory.
Before submitting a pull request, please make sure the following is
done:
1. Fork [the repository](https://github.com/facebook/react) and create
your branch from `main`.
2. Run `yarn` in the repository root.
3. If you've fixed a bug or added code that should be tested, add tests!
4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch
TestName` is helpful in development.
5. Run `yarn test --prod` to test in the production environment. It
supports the same options as `yarn test`.
6. If you need a debugger, run `yarn test --debug --watch TestName`,
open `chrome://inspect`, and press "Inspect".
7. Format your code with
[prettier](https://github.com/prettier/prettier) (`yarn prettier`).
8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only
check changed files.
9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`).
10. If you haven't already, complete the CLA.
Learn more about contributing:
https://reactjs.org/docs/how-to-contribute.html
-->
## Summary
Contributing to https://github.com/facebook/react/pull/32240, this
change adds the tsconfig, tsup config, and estree type declarations that
will be needed for that plugin's typescript migration.
rollup doesn't inline cjs requires (although it can with an external
plugin), so requiring package.json was causing issues internally at Meta
since that file doesn't exist there.
We could teach our build scripts to do so but given that the eslint meta
field is optional anyways I opted to just hardcode the name and omit the
version.
