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

Rename experimental useEvent to useEffectEvent #25881

Merged
merged 2 commits into from
Dec 14, 2022

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Dec 13, 2022

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:

const onHappened = useEffectEvent(() => ...);
useEffect(() => {
  onHappened();
}, []);

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Dec 13, 2022
Copy link
Collaborator

@acdlite acdlite left a comment

Choose a reason for hiding this comment

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

I'll let others weigh in but this name works for me

@poteto
Copy link
Member

poteto commented Dec 13, 2022

No notes about the name, I think it makes sense to me.

It's just internal but just for consistency it might be worth grepping for UseEvent as well (eg enableUseEventHook wasn't renamed) since it looks like there's still a few leftovers that were missed in the rename.

@gaearon
Copy link
Collaborator

gaearon commented Dec 13, 2022

but now it's only meant to be used in combination with effects.

Should it be legit to call it from an event handler?

Edit: I guess you mean yes:

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.

@sizebot
Copy link

sizebot commented Dec 13, 2022

Comparing: 4dda96a...2d97961

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 = 154.58 kB 154.58 kB = 49.01 kB 49.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js +0.02% 156.51 kB 156.53 kB = 49.65 kB 49.65 kB
facebook-www/ReactDOM-prod.classic.js = 533.71 kB 533.75 kB = 95.04 kB 95.04 kB
facebook-www/ReactDOM-prod.modern.js = 518.81 kB 518.85 kB = 92.85 kB 92.85 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against 2d97961

@sebmarkbage sebmarkbage merged commit 84a0a17 into facebook:main Dec 14, 2022
github-actions bot pushed a commit that referenced this pull request Dec 14, 2022
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();
}, []);
```

DiffTrain build for [84a0a17](84a0a17)
[View git log for this commit](https://github.com/facebook/react/commits/84a0a171ea0ecd25e287bd3d3dd30e932beb4677)
@HansBrende
Copy link

@sebmarkbage the RFC's "basic example" is:

function Chat() {
  const [text, setText] = useState('');

  const onClick = useEvent(() => {
    sendMessage(text);
  });

  return <SendButton onClick={onClick} />;
}

Are you saying this won't work anymore because onClick is not called in a useEffect? Or is there something I'm not understanding about the terminology "kind of an advanced case" since this is supposed to be the basic example?

@sebmarkbage
Copy link
Collaborator Author

That RFC is defunct. This is different.

@HansBrende
Copy link

@sebmarkbage to clarify: so we will still need to use use-event-callback for the above use-case I mentioned? useEffectEvent won't cover it?

@gaearon
Copy link
Collaborator

gaearon commented Dec 16, 2022

The scope of the new RFC will be specifically aimed at solving the Effects case. If you find use-event-callback helpful for other reasons you can keep using it for those. We wanted to decouple the Effects problem (which is very clear) from the rendering optimizations (to which there are many possible solutions, and where there isn't a clear winner yet). We will be exploring rendering optimizations separately.

@HansBrende
Copy link

HansBrende commented Dec 17, 2022

@gaearon follow-up question then: I'm trying to write a userland hook that would as closely as possible approximate the original (stable) semantics since they are extremely useful for a lot of projects I'm doing. Do you think this would work (specifically: assigning in render pre-React 18)?

const throwingShim = () => {throw new Error()}

// if concurrent mode is not supported, assign state to ref in render to avoid
// "the pitfall present in the userland versions where one component's effect 
// can observe the previous version of another component's state."
const useCopyStateToRefEffect = React.startTransition !== undefined ? useLayoutEffect : fn => fn();

export const useEvent = 

  // as defined by https://github.com/facebook/react/blob/main/packages/shared/ExecutionEnvironment.js
  !canUseDOM ? 
  
  // "when server rendering, useEvent will return the same throwing shim for all calls"
  () => throwingShim : 
  
  // client implementation
  (fn) => {
    const ref = useRef([fn, (...args) => ref[0](...args)]).current;
    useCopyStateToRefEffect(() => {
        ref[0] = fn;
    });
    return ref[1];
  }

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2022

I would not recommend such drastic behavior differences. An 18-only shim relying on useInsertionEffect seems reasonable to me.

@HansBrende
Copy link

@gaearon thanks for the tip! Something like this? Tweaking the above as follows:

const useCopyStateToRefEffect = React.startTransition !== undefined ? useInsertionEffect : fn => fn();

Note: I can't make it 18-only since I'm working on a library that supports either 17 or 18 as a peer dependency. Do you see any pitfalls with the above approach? It certainly feels like a hack, but I'm scratching my head as to what could possibly break (can't think of anything myself).

@gaearon
Copy link
Collaborator

gaearon commented Dec 17, 2022

If you're using 17 and 18 I'd say just do it in in a layout effect.

@HansBrende
Copy link

@gaearon that's certainly a possibility, but was hoping to avoid "the pitfall present in the userland versions where one component's effect can observe the previous version of another component's state" to the greatest extent possible.

So I'm curious: what other pitfalls do you envision with the approach listed above (using useInsertionEffect with concurrent mode, no effect otherwise)? There must be some reason you are not recommending this approach?

@HansBrende
Copy link

HansBrende commented Dec 17, 2022

@gaearon as an aside: if React itself were to expose some manner of this useCopyStateToRefEffect (either accepting an arbitrary function, or simply a ref and a state), that would be an extremely powerful building block that would make these types of userland hooks actually possible. It would be useful, not just building useEvent hooks, but also, for example, updating Context values that are not supposed to trigger a re-render (e.g. if only used in callbacks). Has this been discussed (and if so, where?) Should I create an issue to propose this?

@karlhorky
Copy link
Contributor

karlhorky commented Dec 23, 2022

@sebmarkbage are there any ideas of a targeted stable version of react for this new useEffectEvent to be included in?

I assume it's not far enough along to be included in react@18.3.0, but maybe something around 18.5.0? Or is this going to require a breaking change and only become available in 19.0.0 or 20.0.0?

@gaearon
Copy link
Collaborator

gaearon commented Dec 23, 2022

Hi @karlhorky, we don't schedule features for concrete minor numbers in the future. We hope to get this out in one of the future minors (maybe the next one) but if we need to release something else in between, it might end up being a different minor. I don't think we currently plan to release a major in between.

facebook-github-bot pushed a commit to facebook/react-native that referenced this pull request Jan 31, 2023
Summary:
This sync includes the following changes:
- **[48b687fc9](facebook/react@48b687fc9 )**: [trusted types][www] Add enableTrustedTypesIntegration flag back in ([#26016](facebook/react#26016)) //<an onion>//
- **[9b1423cc0](facebook/react@9b1423cc0 )**: Revert "Hold host functions in var" ([#26079](facebook/react#26079)) //<Samuel Susla>//
- **[ce09ace9a](facebook/react@ce09ace9a )**: Improve Error Messages when Access Client References ([#26059](facebook/react#26059)) //<Sebastian Markbåge>//
- **[0652bdbd1](facebook/react@0652bdbd1 )**: Add flow types to Maps in ReactNativeViewConfigRegistry.js ([#26064](facebook/react#26064)) //<Samuel Susla>//
- **[ee8509801](facebook/react@ee8509801 )**: [cleanup] remove deletedTreeCleanUpLevel feature flag ([#25529](facebook/react#25529)) //<Jan Kassens>//
- **[0e31dd028](facebook/react@0e31dd028 )**: Remove findDOMNode www shim ([#25998](facebook/react#25998)) //<Jan Kassens>//
- **[379dd741e](facebook/react@379dd741e )**: [www] set enableTrustedTypesIntegration to false ([#25997](facebook/react#25997)) //<Jan Kassens>//
- **[555ece0cd](facebook/react@555ece0cd )**: Don't warn about concurrently rendering contexts if we finished rendering ([#22797](facebook/react#22797)) //<Sebastian Silbermann>//
- **[0fce6bb49](facebook/react@0fce6bb49 )**: [cleanup] remove feature flags warnAboutDefaultPropsOnFunctionComponents and warnAboutStringRefs ([#25980](facebook/react#25980)) //<Jan Kassens>//
- **[7002a6743](facebook/react@7002a6743 )**: [cleanup] remove unused values from ReactFeatureFlags.www-dynamic ([#25575](facebook/react#25575)) //<Jan Kassens>//
- **[a48e54f2b](facebook/react@a48e54f2b )**: [cleanup] remove old feature flag warnAboutDeprecatedLifecycles ([#25978](facebook/react#25978)) //<Jan Kassens>//
- **[0f4a83596](facebook/react@0f4a83596 )**: Remove duplicate JSResourceReferenceImpl mock ([#25976](facebook/react#25976)) //<Jan Kassens>//
- **[c49131669](facebook/react@c49131669 )**: Remove unused Flow suppressions ([#25977](facebook/react#25977)) //<Jan Kassens>//
- **[afe6521](facebook/react@afe6521e1 )**: Refactor: remove useless parameter ([#25923](facebook/react#25923)) //<Chris>//
- **[34464fb16](facebook/react@34464fb16 )**: Upgrade to Flow 0.196.3 ([#25974](facebook/react#25974)) //<Jan Kassens>//
- **[e2424f33b](facebook/react@e2424f33b )**: [flow] enable exact_empty_objects ([#25973](facebook/react#25973)) //<Jan Kassens>//
- **[0b4f44302](facebook/react@0b4f44302 )**: [flow] enable enforce_local_inference_annotations ([#25921](facebook/react#25921)) //<Jan Kassens>//
- **[0b974418c](facebook/react@0b974418c )**: [Fizz] Fork Fizz instruction set for inline script and external runtime ([#25862](facebook/react#25862)) //<mofeiZ>//
- **[5379b6123](facebook/react@5379b6123 )**: Batch sync, default and continuous lanes ([#25700](facebook/react#25700)) //<Tianyu Yao>//
- **[bbf4d2211](facebook/react@bbf4d2211 )**: Update import for babel-code-frame in build script ([#25963](facebook/react#25963)) //<Ming Ye>//
- **[b83baf63f](facebook/react@b83baf63f )**: Transform updates to support Flow this annotation syntax ([#25918](facebook/react#25918)) //<Jan Kassens>//
- **[c2d655207](facebook/react@c2d655207 )**: Unify `use` and `renderDidSuspendDelayIfPossible` implementations ([#25922](facebook/react#25922)) //<Andrew Clark>//
- **[48274a43a](facebook/react@48274a43a )**: Remove vestigial Suspense batching logic ([#25861](facebook/react#25861)) //<Andrew Clark>//
- **[de7d1c907](facebook/react@de7d1c907 )**: Add `fetchPriority` to `<img>` and `<link>` ([#25927](facebook/react#25927)) //<Steven>//
- **[81d4ee9ca](facebook/react@81d4ee9ca )**: reconciler docs: fix small typo - "mode" (instead of "node") ([#25863](facebook/react#25863)) //<satelllte>//
- **[5fcf1a4b4](facebook/react@5fcf1a4b4 )**: Bugfix: Synchronous ping during render phase sometimes unwinds the stack, leading to crash ([#25851](facebook/react#25851)) //<Andrew Clark>//
- **[2b1fb91a5](facebook/react@2b1fb91a5 )**: ESLint upgrade to use hermes-eslint ([#25915](facebook/react#25915)) //<Jan Kassens>//
- **[fabef7a6b](facebook/react@fabef7a6b )**: Resubmit Add HydrationSyncLane ([#25878](facebook/react#25878)) //<Tianyu Yao>//
- **[7efa9e597](facebook/react@7efa9e597 )**: Fix unwinding context during selective hydration ([#25876](facebook/react#25876)) //<Tianyu Yao>//
- **[84a0a171e](facebook/react@84a0a171e )**: Rename experimental useEvent to useEffectEvent ([#25881](facebook/react#25881)) //<Sebastian Markbåge>//
- **[4dda96a40](facebook/react@4dda96a40 )**: [react-www] remove forked bundle ([#25866](facebook/react#25866)) //<Jan Kassens>//
- **[9c09c1cd6](facebook/react@9c09c1cd6 )**: Revert "Fork ReactDOMSharedInternals for www ([#25791](facebook/react#25791))" ([#25864](facebook/react#25864)) //<lauren>//
- **[996e4c0d5](facebook/react@996e4c0d5 )**: Offscreen add attach ([#25603](facebook/react#25603)) //<Samuel Susla>//
- **[b14d7fa4b](facebook/react@b14d7fa4b )**: Add support for setNativeProps to Fabric ([#25737](facebook/react#25737)) //<Samuel Susla>//
- **[819687279](facebook/react@819687279 )**: [Float] Fix typo in ReactDOMResourceValidation.js ([#25798](facebook/react#25798)) //<Ikko Ashimine>//
- **[5dfc485f6](facebook/react@5dfc485f6 )**: fix tests for when float is off ([#25839](facebook/react#25839)) //<Josh Story>//
- **[bfcbf3306](facebook/react@bfcbf3306 )**: toString children of title ([#25838](facebook/react#25838)) //<Sebastian Markbåge>//
- **[d4bc16a7d](facebook/react@d4bc16a7d )**: Revert "[react-www] remove forked bundle" ([#25837](facebook/react#25837)) //<Ricky>//
- **[d69b2cf82](facebook/react@d69b2cf82 )**: [bug fix] revert values in ReactFiberFlags to keep consistency for devtools ([#25832](facebook/react#25832)) //<Mengdi Chen>//
- **[645ae2686](facebook/react@645ae2686 )**: [react-www] remove forked bundle ([#25831](facebook/react#25831)) //<Jan Kassens>//
- **[d807eb52c](facebook/react@d807eb52c )**: Revert recent hydration changes ([#25812](facebook/react#25812)) //<Andrew Clark>//
- **[2ccfa657d](facebook/react@2ccfa657d )**: Fork ReactDOMSharedInternals for www ([#25791](facebook/react#25791)) //<lauren>//
- **[f0534ae94](facebook/react@f0534ae94 )**: Avoid replaying SelectiveHydrationException in dev ([#25754](facebook/react#25754)) //<Tianyu Yao>//
- **[7fab379d8](facebook/react@7fab379d8 )**: fix link to ReactDOMHostconfig in reconciler docs ([#25788](facebook/react#25788)) //<Dmitry>//
- **[500c8aa08](facebook/react@500c8aa08 )**: Add component name to StrictMode error message ([#25718](facebook/react#25718)) //<Samuel Susla>//
- **[353c30252](facebook/react@353c30252 )**: Hold host functions in var ([#25741](facebook/react#25741)) //<Samuel Susla>//

Changelog:
[General][Changed] - React Native sync for revisions 17f6912...48b687f

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D42855483

fbshipit-source-id: c244a595bb2d490a23b333c1b16d04a459ec94fc
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
Summary:
This sync includes the following changes:
- **[48b687fc9](facebook/react@48b687fc9 )**: [trusted types][www] Add enableTrustedTypesIntegration flag back in ([facebook#26016](facebook/react#26016)) //<an onion>//
- **[9b1423cc0](facebook/react@9b1423cc0 )**: Revert "Hold host functions in var" ([facebook#26079](facebook/react#26079)) //<Samuel Susla>//
- **[ce09ace9a](facebook/react@ce09ace9a )**: Improve Error Messages when Access Client References ([facebook#26059](facebook/react#26059)) //<Sebastian Markbåge>//
- **[0652bdbd1](facebook/react@0652bdbd1 )**: Add flow types to Maps in ReactNativeViewConfigRegistry.js ([facebook#26064](facebook/react#26064)) //<Samuel Susla>//
- **[ee8509801](facebook/react@ee8509801 )**: [cleanup] remove deletedTreeCleanUpLevel feature flag ([facebook#25529](facebook/react#25529)) //<Jan Kassens>//
- **[0e31dd028](facebook/react@0e31dd028 )**: Remove findDOMNode www shim ([facebook#25998](facebook/react#25998)) //<Jan Kassens>//
- **[379dd741e](facebook/react@379dd741e )**: [www] set enableTrustedTypesIntegration to false ([facebook#25997](facebook/react#25997)) //<Jan Kassens>//
- **[555ece0cd](facebook/react@555ece0cd )**: Don't warn about concurrently rendering contexts if we finished rendering ([facebook#22797](facebook/react#22797)) //<Sebastian Silbermann>//
- **[0fce6bb49](facebook/react@0fce6bb49 )**: [cleanup] remove feature flags warnAboutDefaultPropsOnFunctionComponents and warnAboutStringRefs ([facebook#25980](facebook/react#25980)) //<Jan Kassens>//
- **[7002a6743](facebook/react@7002a6743 )**: [cleanup] remove unused values from ReactFeatureFlags.www-dynamic ([facebook#25575](facebook/react#25575)) //<Jan Kassens>//
- **[a48e54f2b](facebook/react@a48e54f2b )**: [cleanup] remove old feature flag warnAboutDeprecatedLifecycles ([facebook#25978](facebook/react#25978)) //<Jan Kassens>//
- **[0f4a83596](facebook/react@0f4a83596 )**: Remove duplicate JSResourceReferenceImpl mock ([facebook#25976](facebook/react#25976)) //<Jan Kassens>//
- **[c49131669](facebook/react@c49131669 )**: Remove unused Flow suppressions ([facebook#25977](facebook/react#25977)) //<Jan Kassens>//
- **[afe6521](facebook/react@afe6521e1 )**: Refactor: remove useless parameter ([facebook#25923](facebook/react#25923)) //<Chris>//
- **[34464fb16](facebook/react@34464fb16 )**: Upgrade to Flow 0.196.3 ([facebook#25974](facebook/react#25974)) //<Jan Kassens>//
- **[e2424f33b](facebook/react@e2424f33b )**: [flow] enable exact_empty_objects ([facebook#25973](facebook/react#25973)) //<Jan Kassens>//
- **[0b4f44302](facebook/react@0b4f44302 )**: [flow] enable enforce_local_inference_annotations ([facebook#25921](facebook/react#25921)) //<Jan Kassens>//
- **[0b974418c](facebook/react@0b974418c )**: [Fizz] Fork Fizz instruction set for inline script and external runtime ([facebook#25862](facebook/react#25862)) //<mofeiZ>//
- **[5379b6123](facebook/react@5379b6123 )**: Batch sync, default and continuous lanes ([facebook#25700](facebook/react#25700)) //<Tianyu Yao>//
- **[bbf4d2211](facebook/react@bbf4d2211 )**: Update import for babel-code-frame in build script ([facebook#25963](facebook/react#25963)) //<Ming Ye>//
- **[b83baf63f](facebook/react@b83baf63f )**: Transform updates to support Flow this annotation syntax ([facebook#25918](facebook/react#25918)) //<Jan Kassens>//
- **[c2d655207](facebook/react@c2d655207 )**: Unify `use` and `renderDidSuspendDelayIfPossible` implementations ([facebook#25922](facebook/react#25922)) //<Andrew Clark>//
- **[48274a43a](facebook/react@48274a43a )**: Remove vestigial Suspense batching logic ([facebook#25861](facebook/react#25861)) //<Andrew Clark>//
- **[de7d1c907](facebook/react@de7d1c907 )**: Add `fetchPriority` to `<img>` and `<link>` ([facebook#25927](facebook/react#25927)) //<Steven>//
- **[81d4ee9ca](facebook/react@81d4ee9ca )**: reconciler docs: fix small typo - "mode" (instead of "node") ([facebook#25863](facebook/react#25863)) //<satelllte>//
- **[5fcf1a4b4](facebook/react@5fcf1a4b4 )**: Bugfix: Synchronous ping during render phase sometimes unwinds the stack, leading to crash ([facebook#25851](facebook/react#25851)) //<Andrew Clark>//
- **[2b1fb91a5](facebook/react@2b1fb91a5 )**: ESLint upgrade to use hermes-eslint ([facebook#25915](facebook/react#25915)) //<Jan Kassens>//
- **[fabef7a6b](facebook/react@fabef7a6b )**: Resubmit Add HydrationSyncLane ([facebook#25878](facebook/react#25878)) //<Tianyu Yao>//
- **[7efa9e597](facebook/react@7efa9e597 )**: Fix unwinding context during selective hydration ([facebook#25876](facebook/react#25876)) //<Tianyu Yao>//
- **[84a0a171e](facebook/react@84a0a171e )**: Rename experimental useEvent to useEffectEvent ([facebook#25881](facebook/react#25881)) //<Sebastian Markbåge>//
- **[4dda96a40](facebook/react@4dda96a40 )**: [react-www] remove forked bundle ([facebook#25866](facebook/react#25866)) //<Jan Kassens>//
- **[9c09c1cd6](facebook/react@9c09c1cd6 )**: Revert "Fork ReactDOMSharedInternals for www ([facebook#25791](facebook/react#25791))" ([facebook#25864](facebook/react#25864)) //<lauren>//
- **[996e4c0d5](facebook/react@996e4c0d5 )**: Offscreen add attach ([facebook#25603](facebook/react#25603)) //<Samuel Susla>//
- **[b14d7fa4b](facebook/react@b14d7fa4b )**: Add support for setNativeProps to Fabric ([facebook#25737](facebook/react#25737)) //<Samuel Susla>//
- **[819687279](facebook/react@819687279 )**: [Float] Fix typo in ReactDOMResourceValidation.js ([facebook#25798](facebook/react#25798)) //<Ikko Ashimine>//
- **[5dfc485f6](facebook/react@5dfc485f6 )**: fix tests for when float is off ([facebook#25839](facebook/react#25839)) //<Josh Story>//
- **[bfcbf3306](facebook/react@bfcbf3306 )**: toString children of title ([facebook#25838](facebook/react#25838)) //<Sebastian Markbåge>//
- **[d4bc16a7d](facebook/react@d4bc16a7d )**: Revert "[react-www] remove forked bundle" ([facebook#25837](facebook/react#25837)) //<Ricky>//
- **[d69b2cf82](facebook/react@d69b2cf82 )**: [bug fix] revert values in ReactFiberFlags to keep consistency for devtools ([facebook#25832](facebook/react#25832)) //<Mengdi Chen>//
- **[645ae2686](facebook/react@645ae2686 )**: [react-www] remove forked bundle ([facebook#25831](facebook/react#25831)) //<Jan Kassens>//
- **[d807eb52c](facebook/react@d807eb52c )**: Revert recent hydration changes ([facebook#25812](facebook/react#25812)) //<Andrew Clark>//
- **[2ccfa657d](facebook/react@2ccfa657d )**: Fork ReactDOMSharedInternals for www ([facebook#25791](facebook/react#25791)) //<lauren>//
- **[f0534ae94](facebook/react@f0534ae94 )**: Avoid replaying SelectiveHydrationException in dev ([facebook#25754](facebook/react#25754)) //<Tianyu Yao>//
- **[7fab379d8](facebook/react@7fab379d8 )**: fix link to ReactDOMHostconfig in reconciler docs ([facebook#25788](facebook/react#25788)) //<Dmitry>//
- **[500c8aa08](facebook/react@500c8aa08 )**: Add component name to StrictMode error message ([facebook#25718](facebook/react#25718)) //<Samuel Susla>//
- **[353c30252](facebook/react@353c30252 )**: Hold host functions in var ([facebook#25741](facebook/react#25741)) //<Samuel Susla>//

Changelog:
[General][Changed] - React Native sync for revisions 17f6912...48b687f

jest_e2e[run_all_tests]

Reviewed By: rubennorte

Differential Revision: D42855483

fbshipit-source-id: c244a595bb2d490a23b333c1b16d04a459ec94fc
@michaelphines-stripe
Copy link

michaelphines-stripe commented May 26, 2023

This meshes with my intuition that often the underlying issue with useCallback deps is actually an issue with deps within useEffect of the component the callback is passed as a prop to. However, I think if useEffectEvent is constrained to usages within useEffect, the implementation can be much simpler than the previous useEvent.

The reason is that useEffect already has the up-to-date closure semantics of useReducer referenced in the original 2018 issue that useEvent was trying to replicate. The implementation details of useEvent were specific to the useCallback use case, which was special because React is not in control of when the callback is run. When constrained to usage within effects of the same components, the extra details are unnecessary.

These two rules together are what simplify things for us:

  • Only call Effect Events from inside Effects.
  • Don’t pass Effect Events to other components or Hooks.

It seems to me that if you follow all the rules of useEffectEvent, what you are doing is tantamount to just ignoring a dep. In fact, useEffectEvent needs no implementation at all! All useEffectEvent needs to do is return its argument and exist as a hint to the linter to ignore the missing dep. It may also have its own linter rules. However, if it's ever anything more than that, it's just providing unnecessary functionality that is only useful when misused.

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.

9 participants