-
Couldn't load subscription status.
- Fork 1.3k
feat: support modern useEffectEvent #9095
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR
| }, []); | ||
| } | ||
|
|
||
| export function useEffectEvent<T extends Function>(fn?: T): T { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure why fn was optional.... this is an internal hook for us (not documented), so I think we should probably have it be required, then we can get rid of this extra noop function
|
Doh, I see we were passing it possibly undefined in utils/src/useEvent.ts, but we should apply your noop specifically there i think. Looks like that hook very specifically allowed for it. |
|
@snowystinger I'm fine with requiring the argument, but wanted to note that we definitely lose some convenience. With the upgrade to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@snowystinger I'm fine with requiring the argument, but wanted to note that we definitely loose some convenience. With the upgrade to
rules-of-hooksyou recently performed, the recommended pattern to opt out of reactivity is to wrap callbacks passed via props withuseEffectEvent. Often times props are optional, which would require us to provide a noop, making this pretty cumbersome.
Yeah, I see that, though it looks like we've only needed it in one place out of all of our usages. So I'm ok with it, we'll see what the rest of the team thinks.
Thanks again!
This PR adds compatibility for React's new
useEffectEventhook, with fallback to our legacy shim.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: