## 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.
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>
<!--
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
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
Currently, `react-hooks/rules-of-hooks` does not support `do/while`
loops - I've also reported this in
https://github.com/facebook/react/issues/28713.
This PR takes a stab at adding support for `do/while` by following the
same logic we already have for detecting `while` loops.
After this PR, any hooks called inside a `do/while` loop will be
considered invalid.
We're also adding some unit tests to confirm that the behavior is
working as expected.
Fixes#28713.
## How did you test this change?
<!--
Demonstrate the code is solid. Example: The exact commands you ran and
their output, screenshots / videos if the pull request changes the user
interface.
How exactly did you verify that your PR solves the issue you wanted to
solve?
If you leave this empty, your PR will very likely be closed.
-->
I've added unit tests that cover the case and verified that they pass by
running:
```
yarn test packages/eslint-plugin-react-hooks/__tests__/ESLintRulesOfHooks-test.js --watch
```
I've also verified that the rest of the tests continue to pass by
running:
```
yarn test
```
and
```
yarn test --prod
```
## Summary
This PR closes#25844
The original issue talks about `as const`, but seems like it fails for
any `as X` expressions since it adds another nesting level to the AST.
EDIT: Also closes#20162
## How did you test this change?
Added unit tests
We should probably treat `React.use()` the same as `use()` to allow it
within loops and conditionals.
Ideally this would implement a test that `React` is imported or required
from `'react'`, but we don't otherwise implement such a test.
## Summary
There is a bug in the `react-hooks/exhaustive-deps` rule that forbids
the dependencies argument from being `undefined`. It triggers the error
that the dependency list is not an array literal. This makes sense in
pre ES5 strict-mode environments as undefined could be redefined, but
should not be a concern in today's JS environments.
**Justification:**
* The deps argument being undefined (for `useEffect` calls etc.) is a
valid use case for hooks that should re-run on every render.
* The deps argument being omitted is considered a valid use case by the
`exhaustive-deps` rule already.
* The TypeScript type definitions support passing `undefined` because
hooks are typed as `useEffect(effect: EffectCallback, deps?:
DependencyList): void;`.
* Since omitting an argument and passing `undefined` are considered
equivalent, this eslint rule should consider them as equivalent too.
Further, I accidentally forgot passing a dependency array to `useEffect`
in code that I shared on Twitter, and people started abusing me about
it. I'd like to create an eslint rule for my projects that requires me
to provide a dep argument in all cases (`undefined`, `[]` or the list of
dependencies) so that I can avoid such problems in the future. This
would also force me to always think about the dependencies instead of
accidentally forgetting them and my hook running on each render. In an
audit of my own codebase I had about 3% of hooks that I want to run on
each render, and adding an explicit `undefined` seems reasonable in
those situations.
It could be argued this could be an option or part of the
`exhaustive-deps` rule, but it's probably better to merge this PR, make
a release and see if my custom eslint rule gains traction in the future.
## How did you test this change?
* Added a test.
* `yarn test ESLintRuleExhaustiveDeps-test`
* Careful code inspection.
Hooks cannot be called in async functions, on either the client or the
server. This mistake sometimes happens when using Server Components,
especially when refactoring a Server Component to a Client Component.
React logs a warning at runtime, but it's even better to catch this with
a lint rule since it will show immediate inline feedback in the editor.
I added this to the existing "Rules of Hooks" ESLint rule.
- substr is Annex B
- substring silently flips its arguments if they're in the "wrong order", which is confusing
- slice is better than sliced bread (no pun intended) and also it works the same way on Arrays so there's less to remember
---
> I'd be down to just lint and enforce a single form just for the potential compression savings by using a repeated string.
_Originally posted by @sebmarkbage in https://github.com/facebook/react/pull/26663#discussion_r1170455401_
The old version of prettier we were using didn't support the Flow syntax
to access properties in a type using `SomeType['prop']`. This updates
`prettier` and `rollup-plugin-prettier` to the latest versions.
I added the prettier config `arrowParens: "avoid"` to reduce the diff
size as the default has changed in Prettier 2.0. The largest amount of
changes comes from function expressions now having a space. This doesn't
have an option to preserve the old behavior, so we have to update this.
We originally had grand plans for using this Event concept for more but
now it's only meant to be used in combination with effects.
It's an Event in the FRP terms, that is triggered from an Effect.
Technically it can also be from another function that itself is
triggered from an existing side-effect but that's kind of an advanced
case.
The canonical case is an effect that triggers an event:
```js
const onHappened = useEffectEvent(() => ...);
useEffect(() => {
onHappened();
}, []);
```
* [useEvent] Lint for presence of useEvent functions in dependency lists
With #25473, the identity of useEvent's return value is no longer stable
across renders. Previously, the ExhaustiveDeps lint rule would only
allow the omission of the useEvent function, but you could still add it
as a dependency.
This PR updates the ExhaustiveDeps rule to explicitly check for the
presence of useEvent functions in dependency lists, and emits a warning
and suggestion/autofixer for removing the dependency.
* Facebook -> Meta in copyright
rg --files | xargs sed -i 's#Copyright (c) Facebook, Inc. and its affiliates.#Copyright (c) Meta Platforms, Inc. and affiliates.#g'
* Manual tweaks
Usage of the new `use` hook needs to conform to the rules of hooks, with
the one exception that it can be called conditionally.
ghstack-source-id: 7ea5beceaf
Pull Request resolved: https://github.com/facebook/react/pull/25370
* [ESLint] Check useEvent references instead
Previously the useEvent check in RulesOfHooks would collect all
definitions of useEvent functions at the top level, record them as
violations, then clear those violations if the useEvent function was
later called or referened inside of an effect or another event.
The flaw with this approach was in the special case where useEvent
functions could be passed by reference inside of effects or events. The
violation would be cleared here (since it was called at least once)
and subsequent usages of the useEvent function would not be properly
checked.
This PR changes it so we check all identifiers that resolve to a
useEvent function, and if they are not in an effect or event must be
called or a lint error is emitted.
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
* Add comment
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
This update to the RulesOfHooks rule checks that functions created with
`useEvent` can only be invoked in a `useEffect` callback, in another
event function, or a closure.
They can't be passed down directly as a reference to child components.
This PR also updates the ExhaustiveDeps lint rule to treat useEvent's
return value as stable, so it can be omitted from dependency lists.
Currently this all gated behind an experimental flag.
Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
* Switched RulesOfHooks.js to use BigInt. Added test and updated .eslintrc.js to use es2020.
* Added BigInt as readonly global in eslintrc.cjs.js and eslintrc.cjs2015.js
* Added comment to RulesOfHooks.js that gets rid of BigInt eslint error
* Got rid of changes in .eslintrc.js and yarn.lock
* Move global down
Co-authored-by: stephen cyron <stephen.cyron@fdmgroup.com>
Co-authored-by: dan <dan.abramov@gmail.com>
