Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[useEvent] Non-stable function identity #25473

Merged
merged 5 commits into from
Oct 19, 2022
Merged

[useEvent] Non-stable function identity #25473

merged 5 commits into from
Oct 19, 2022

Conversation

poteto
Copy link
Member

@poteto poteto commented Oct 12, 2022

Since useEvent shouldn't go in the dependency list of whatever is consuming it (which is enforced by the fact that useEvent functions are always locally created and never passed by reference), its identity doesn't matter. Effectively, this PR is a runtime assertion that you can't rely on the return value of useEvent to be stable.

PR to ExhaustiveDeps to properly lint for this: #25512

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 12, 2022
@sizebot
Copy link

sizebot commented Oct 12, 2022

Comparing: 9872928...c8ab997

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 136.01 kB 136.01 kB = 43.55 kB 43.54 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.06% 151.48 kB 151.57 kB +0.03% 48.24 kB 48.26 kB
facebook-www/ReactDOM-prod.classic.js +0.03% 493.67 kB 493.79 kB = 87.87 kB 87.87 kB
facebook-www/ReactDOM-prod.modern.js +0.03% 478.92 kB 479.05 kB = 85.69 kB 85.69 kB
facebook-www/ReactDOMForked-prod.classic.js +0.03% 493.67 kB 493.79 kB = 87.87 kB 87.87 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against c8ab997

@poteto poteto force-pushed the lt/useEvent-non-stable branch 6 times, most recently from 1730a42 to 62bd1ef Compare October 19, 2022 15:26
poteto and others added 3 commits October 19, 2022 11:28
Since useEvent shouldn't go in the dependency list of whatever is
consuming it (which is enforced by the fact that useEvent functions are
always locally created and never passed by reference), its identity
doesn't matter. Effectively, this PR is a runtime assertion
that you can't rely on the return value of useEvent to be stable.
The key feature of useEvent that makes it different from useCallback
is that events always see the latest committed values. There's no such
thing as a "stale" event handler.
poteto added a commit that referenced this pull request Oct 19, 2022
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.
callback: F,
): EventFunctionWrapper<Args, Return, F> {
const hook = mountWorkInProgressHook();
const eventFn = wrapEventFunction(callback);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Now that we don't reuse the eventFn anymore, there's not any advantage to using it in place of a ref-style mutable object. So I would change this back to something like {impl: eventFn} to avoid confusion.

Also I'm noticing now that during initial render, you don't need to schedule a commit effect. It's already initialized to the correct implementation.

- Inlines wrapping of the callback
- Use a mutable ref-style object instead of a callable object
- Fix types
@poteto poteto merged commit 3cc792b into main Oct 19, 2022
@poteto poteto deleted the lt/useEvent-non-stable branch October 19, 2022 18:59
poteto added a commit that referenced this pull request Oct 19, 2022
#25512)

* [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-github-bot pushed a commit to facebook/react-native that referenced this pull request Nov 1, 2022
Summary:
This sync includes the following changes:
- **[ab075a232](facebook/react@ab075a232 )**: Do not unmount layout effects on initial Offscreen mount ([#25592](facebook/react#25592)) //<Samuel Susla>//
- **[765805bf8](facebook/react@765805bf8 )**: Fix type check for null ([#25595](facebook/react#25595)) //<Sebastian Markbåge>//
- **[2ac77aab9](facebook/react@2ac77aab9 )**: Clean up vestige of useOpaqueIdentifier ([#25587](facebook/react#25587)) //<Andrew Clark>//
- **[bdd3d0807](facebook/react@bdd3d0807 )**: Extract logic for detecting bad fallback to helper //<Andrew Clark>//
- **[952dfff3f](facebook/react@952dfff3f )**: Split suspended work loop logic into separate functions //<Andrew Clark>//
- **[d2c0ab10d](facebook/react@d2c0ab10d )**: In work loop, add enum of reasons for suspending //<Andrew Clark>//
- **[5450dd409](facebook/react@5450dd409 )**: Strict Mode: Reuse memoized result from first pass ([#25583](facebook/react#25583)) //<Andrew Clark>//
- **[d2a0176a1](facebook/react@d2a0176a1 )**: Detect and warn if use(promise) is wrapped with try/catch block ([#25543](facebook/react#25543)) //<Andrew Clark>//
- **[cf3932be5](facebook/react@cf3932be5 )**: Remove old react-fetch, react-fs and react-pg libraries ([#25577](facebook/react#25577)) //<Sebastian Markbåge>//
- **[28a574ea8](facebook/react@28a574ea8 )**: Try assigning fetch to globalThis if global assignment fails ([#25571](facebook/react#25571)) //<Sebastian Markbåge>//
- **[09def5990](facebook/react@09def5990 )**: [Float] handle noscript context for Resources ([#25559](facebook/react#25559)) //<Josh Story>//
- **[17204056d](facebook/react@17204056d )**: [Float] fix coordination of resource identity and hydration ([#25569](facebook/react#25569)) //<Josh Story>//
- **[d925a8d0b](facebook/react@d925a8d0b )**: Flight client error stack ([#25560](facebook/react#25560)) //<Josh Story>//
- **[996b00b78](facebook/react@996b00b78 )**: [Tiny] Fixed incorrect import in `react-server-dom-webpack` ([#25554](facebook/react#25554)) //<Leo Lamprecht>//
- **[e7c5af45c](facebook/react@e7c5af45c )**: Update cache() and use() to the canary aka next channel ([#25502](facebook/react#25502)) //<Sebastian Markbåge>//
- **[fa77f52e7](facebook/react@fa77f52e7 )**: Unify promise switch statements //<Andrew Clark>//
- **[7572e4931](facebook/react@7572e4931 )**: Track thenable state in work loop //<Andrew Clark>//
- **[7fc3eefd8](facebook/react@7fc3eefd8 )**: Revert yieldy behavior for non-use Suspense (in Flight, too) //<Andrew Clark>//
- **[61f9b5e97](facebook/react@61f9b5e97 )**: [Float] support <base> as Resource ([#25546](facebook/react#25546)) //<Josh Story>//
- **[1d3fc9c9c](facebook/react@1d3fc9c9c )**: Bug fix when resolving cache ([#25545](facebook/react#25545)) //<Sebastian Markbåge>//
- **[cce18e350](facebook/react@cce18e350 )**: [Flight] Use AsyncLocalStorage to extend the scope of the cache to micro tasks ([#25542](facebook/react#25542)) //<Sebastian Markbåge>//
- **[caa84c8da](facebook/react@caa84c8da )**: Revert fetch instrumentation to only RSC ([#25540](facebook/react#25540)) //<Sebastian Markbåge>//
- **[0c11baa6a](facebook/react@0c11baa6a )**: add warnings for non-resources rendered outside body or head ([#25532](facebook/react#25532)) //<Josh Story>//
- **[9236abdb5](facebook/react@9236abdb5 )**: when float is enabled only push title and script as a single unit ([#25536](facebook/react#25536)) //<Josh Story>//
- **[dd5c20825](facebook/react@dd5c20825 )**: Revert yieldy behavior for non-use Suspense ([#25537](facebook/react#25537)) //<Andrew Clark>//
- **[934177598](facebook/react@934177598 )**: fix transposed escape functions ([#25534](facebook/react#25534)) //<Josh Story>//
- **[d1ced9fd5](facebook/react@d1ced9fd5 )**: [Float] support all links as Resources ([#25515](facebook/react#25515)) //<Josh Story>//
- **[973b90bdf](facebook/react@973b90bdf )**: [Float] support meta tags as Resources ([#25514](facebook/react#25514)) //<Josh Story>//
- **[79c582981](facebook/react@79c582981 )**: Let ReactDOM initialize in RSC ([#25503](facebook/react#25503)) //<Sebastian Markbåge>//
- **[1f7a2f577](facebook/react@1f7a2f577 )**: [Float] support title tags as Resources ([#25508](facebook/react#25508)) //<Josh Story>//
- **[c63580787](facebook/react@c63580787 )**: Support `use` in `act` testing API ([#25523](facebook/react#25523)) //<Andrew Clark>//
- **[65e32e58b](facebook/react@65e32e58b )**: Add fetch Instrumentation to Dedupe Fetches ([#25516](facebook/react#25516)) //<Sebastian Markbåge>//
- **[9336e29d9](facebook/react@9336e29d9 )**: [useEvent] Lint for presence of useEvent functions in dependency lists ([#25512](facebook/react#25512)) //<lauren>//
- **[3cc792bfb](facebook/react@3cc792bfb )**: [useEvent] Non-stable function identity ([#25473](facebook/react#25473)) //<lauren>//
- **[987292815](facebook/react@987292815 )**: Remove feature flag enableStrictEffects ([#25387](facebook/react#25387)) //<Samuel Susla>//
- **[8e2bde6f2](facebook/react@8e2bde6f2 )**: Add cache() API ([#25506](facebook/react#25506)) //<Sebastian Markbåge>//
- **[9cdf8a99e](facebook/react@9cdf8a99e )**: [Codemod] Update copyright header to Meta ([#25315](facebook/react#25315)) //<Andrew Clark>//
- **[e54015e26](facebook/react@e54015e26 )**: Refactor: fill in the flow missing type ([#25496](facebook/react#25496)) //<c0dedance>//
- **[3b1fd5767](facebook/react@3b1fd5767 )**: refactor: Flow: typing of Scheduler ([#25485](facebook/react#25485)) //<bubucuo>//
- **[14072ce64](facebook/react@14072ce64 )**: Add detach to Offscreen component ([#25265](facebook/react#25265)) //<Samuel Susla>//
- **[3bb71dfd4](facebook/react@3bb71dfd4 )**: Rename react-server-dom-webpack entry points to /client and /server ([#25504](facebook/react#25504)) //<Sebastian Markbåge>//
- **[71f2c8cf1](facebook/react@71f2c8cf1 )**: move resource acquisition to mutation phase ([#25500](facebook/react#25500)) //<Josh Story>//
- **[500bea532](facebook/react@500bea532 )**: Add option to load Fizz runtime from external file ([#25499](facebook/react#25499)) //<Andrew Clark>//
- **[4494f2a86](facebook/react@4494f2a86 )**: [Float] add support for scripts and other enhancements ([#25480](facebook/react#25480)) //<Josh Story>//
- **[9ecf84ed7](facebook/react@9ecf84ed7 )**: Bugfix: Suspending in shell during discrete update ([#25495](facebook/react#25495)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 54f297a...ab075a2

jest_e2e[run_all_tests]

Reviewed By: kassens

Differential Revision: D40897093

fbshipit-source-id: 6a040315834dea5c0ab994ea94d91f5605b9d6b0
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
* [useEvent] Non-stable function identity

Since useEvent shouldn't go in the dependency list of whatever is
consuming it (which is enforced by the fact that useEvent functions are
always locally created and never passed by reference), its identity
doesn't matter. Effectively, this PR is a runtime assertion
that you can't rely on the return value of useEvent to be stable.

* Test: Events should see latest bindings

The key feature of useEvent that makes it different from useCallback
is that events always see the latest committed values. There's no such
thing as a "stale" event handler.

* Don't queue a commit effect on mount

* Inline event function wrapping

- Inlines wrapping of the callback
- Use a mutable ref-style object instead of a callable object
- Fix types

Co-authored-by: Andrew Clark <git@andrewclark.io>
rickhanlonii pushed a commit that referenced this pull request Dec 3, 2022
#25512)

* [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.
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[ab075a232](facebook/react@ab075a232 )**: Do not unmount layout effects on initial Offscreen mount ([facebook#25592](facebook/react#25592)) //<Samuel Susla>//
- **[765805bf8](facebook/react@765805bf8 )**: Fix type check for null ([facebook#25595](facebook/react#25595)) //<Sebastian Markbåge>//
- **[2ac77aab9](facebook/react@2ac77aab9 )**: Clean up vestige of useOpaqueIdentifier ([facebook#25587](facebook/react#25587)) //<Andrew Clark>//
- **[bdd3d0807](facebook/react@bdd3d0807 )**: Extract logic for detecting bad fallback to helper //<Andrew Clark>//
- **[952dfff3f](facebook/react@952dfff3f )**: Split suspended work loop logic into separate functions //<Andrew Clark>//
- **[d2c0ab10d](facebook/react@d2c0ab10d )**: In work loop, add enum of reasons for suspending //<Andrew Clark>//
- **[5450dd409](facebook/react@5450dd409 )**: Strict Mode: Reuse memoized result from first pass ([facebook#25583](facebook/react#25583)) //<Andrew Clark>//
- **[d2a0176a1](facebook/react@d2a0176a1 )**: Detect and warn if use(promise) is wrapped with try/catch block ([facebook#25543](facebook/react#25543)) //<Andrew Clark>//
- **[cf3932be5](facebook/react@cf3932be5 )**: Remove old react-fetch, react-fs and react-pg libraries ([facebook#25577](facebook/react#25577)) //<Sebastian Markbåge>//
- **[28a574ea8](facebook/react@28a574ea8 )**: Try assigning fetch to globalThis if global assignment fails ([facebook#25571](facebook/react#25571)) //<Sebastian Markbåge>//
- **[09def5990](facebook/react@09def5990 )**: [Float] handle noscript context for Resources ([facebook#25559](facebook/react#25559)) //<Josh Story>//
- **[17204056d](facebook/react@17204056d )**: [Float] fix coordination of resource identity and hydration ([facebook#25569](facebook/react#25569)) //<Josh Story>//
- **[d925a8d0b](facebook/react@d925a8d0b )**: Flight client error stack ([facebook#25560](facebook/react#25560)) //<Josh Story>//
- **[996b00b78](facebook/react@996b00b78 )**: [Tiny] Fixed incorrect import in `react-server-dom-webpack` ([facebook#25554](facebook/react#25554)) //<Leo Lamprecht>//
- **[e7c5af45c](facebook/react@e7c5af45c )**: Update cache() and use() to the canary aka next channel ([facebook#25502](facebook/react#25502)) //<Sebastian Markbåge>//
- **[fa77f52e7](facebook/react@fa77f52e7 )**: Unify promise switch statements //<Andrew Clark>//
- **[7572e4931](facebook/react@7572e4931 )**: Track thenable state in work loop //<Andrew Clark>//
- **[7fc3eefd8](facebook/react@7fc3eefd8 )**: Revert yieldy behavior for non-use Suspense (in Flight, too) //<Andrew Clark>//
- **[61f9b5e97](facebook/react@61f9b5e97 )**: [Float] support <base> as Resource ([facebook#25546](facebook/react#25546)) //<Josh Story>//
- **[1d3fc9c9c](facebook/react@1d3fc9c9c )**: Bug fix when resolving cache ([facebook#25545](facebook/react#25545)) //<Sebastian Markbåge>//
- **[cce18e350](facebook/react@cce18e350 )**: [Flight] Use AsyncLocalStorage to extend the scope of the cache to micro tasks ([facebook#25542](facebook/react#25542)) //<Sebastian Markbåge>//
- **[caa84c8da](facebook/react@caa84c8da )**: Revert fetch instrumentation to only RSC ([facebook#25540](facebook/react#25540)) //<Sebastian Markbåge>//
- **[0c11baa6a](facebook/react@0c11baa6a )**: add warnings for non-resources rendered outside body or head ([facebook#25532](facebook/react#25532)) //<Josh Story>//
- **[9236abdb5](facebook/react@9236abdb5 )**: when float is enabled only push title and script as a single unit ([facebook#25536](facebook/react#25536)) //<Josh Story>//
- **[dd5c20825](facebook/react@dd5c20825 )**: Revert yieldy behavior for non-use Suspense ([facebook#25537](facebook/react#25537)) //<Andrew Clark>//
- **[934177598](facebook/react@934177598 )**: fix transposed escape functions ([facebook#25534](facebook/react#25534)) //<Josh Story>//
- **[d1ced9fd5](facebook/react@d1ced9fd5 )**: [Float] support all links as Resources ([facebook#25515](facebook/react#25515)) //<Josh Story>//
- **[973b90bdf](facebook/react@973b90bdf )**: [Float] support meta tags as Resources ([facebook#25514](facebook/react#25514)) //<Josh Story>//
- **[79c582981](facebook/react@79c582981 )**: Let ReactDOM initialize in RSC ([facebook#25503](facebook/react#25503)) //<Sebastian Markbåge>//
- **[1f7a2f577](facebook/react@1f7a2f577 )**: [Float] support title tags as Resources ([facebook#25508](facebook/react#25508)) //<Josh Story>//
- **[c63580787](facebook/react@c63580787 )**: Support `use` in `act` testing API ([facebook#25523](facebook/react#25523)) //<Andrew Clark>//
- **[65e32e58b](facebook/react@65e32e58b )**: Add fetch Instrumentation to Dedupe Fetches ([facebook#25516](facebook/react#25516)) //<Sebastian Markbåge>//
- **[9336e29d9](facebook/react@9336e29d9 )**: [useEvent] Lint for presence of useEvent functions in dependency lists ([facebook#25512](facebook/react#25512)) //<lauren>//
- **[3cc792bfb](facebook/react@3cc792bfb )**: [useEvent] Non-stable function identity ([facebook#25473](facebook/react#25473)) //<lauren>//
- **[987292815](facebook/react@987292815 )**: Remove feature flag enableStrictEffects ([facebook#25387](facebook/react#25387)) //<Samuel Susla>//
- **[8e2bde6f2](facebook/react@8e2bde6f2 )**: Add cache() API ([facebook#25506](facebook/react#25506)) //<Sebastian Markbåge>//
- **[9cdf8a99e](facebook/react@9cdf8a99e )**: [Codemod] Update copyright header to Meta ([facebook#25315](facebook/react#25315)) //<Andrew Clark>//
- **[e54015e26](facebook/react@e54015e26 )**: Refactor: fill in the flow missing type ([facebook#25496](facebook/react#25496)) //<c0dedance>//
- **[3b1fd5767](facebook/react@3b1fd5767 )**: refactor: Flow: typing of Scheduler ([facebook#25485](facebook/react#25485)) //<bubucuo>//
- **[14072ce64](facebook/react@14072ce64 )**: Add detach to Offscreen component ([facebook#25265](facebook/react#25265)) //<Samuel Susla>//
- **[3bb71dfd4](facebook/react@3bb71dfd4 )**: Rename react-server-dom-webpack entry points to /client and /server ([facebook#25504](facebook/react#25504)) //<Sebastian Markbåge>//
- **[71f2c8cf1](facebook/react@71f2c8cf1 )**: move resource acquisition to mutation phase ([facebook#25500](facebook/react#25500)) //<Josh Story>//
- **[500bea532](facebook/react@500bea532 )**: Add option to load Fizz runtime from external file ([facebook#25499](facebook/react#25499)) //<Andrew Clark>//
- **[4494f2a86](facebook/react@4494f2a86 )**: [Float] add support for scripts and other enhancements ([facebook#25480](facebook/react#25480)) //<Josh Story>//
- **[9ecf84ed7](facebook/react@9ecf84ed7 )**: Bugfix: Suspending in shell during discrete update ([facebook#25495](facebook/react#25495)) //<Andrew Clark>//

Changelog:
[General][Changed] - React Native sync for revisions 54f297a...ab075a2

jest_e2e[run_all_tests]

Reviewed By: kassens

Differential Revision: D40897093

fbshipit-source-id: 6a040315834dea5c0ab994ea94d91f5605b9d6b0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants