-
Notifications
You must be signed in to change notification settings - Fork 559
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
RFC: useEvent #220
RFC: useEvent #220
Conversation
Finally something easy to think of is implemented as it seems. 👍🏻 |
Great idea ❤️ My main suggestion would be a different name. |
Or maybe just name this Unless someone can figure out any valid use case for the old |
As for the name, IMHO, react hook is never named for tech purpose, more in the business direction. If i really want to be picky, i can think of |
I've been using this all over the place. I call it I don't like
I think this would be a better change. I almost always want to use As Merri said, |
Great to see a proposal for this! I'm left wondering though what type of footguns you have identified related to using event functions during render? Out of date callbacks?
If calling event handlers during rendering was allowed, then would the event handler be up to date or not? I.e. would const [value, setValue] = useState(0);
const getCurrentValue = useEvent(() => value);
// always true or sometimes false?
const isCurrent = value === getCurrentValue(); |
I didn't like the useEvent naming too. maybe split it into two different hooks.
|
To be implementation-clear, the behaviour proposed here is not the same as this, right? // NOT the proposal? Since it would have stale props
const useEvent = (cb) => {
const wrapper = useCallback(cb, []);
return wrapper;
};
It'd be more like this (+some err handling), which means the wrapper is always stable but inside keeps the latest ref to the callback and calls that when invoked: const useEvent = (cb) => {
const ref = useRef(cb);
useEffect(() => {
ref.current = cb;
}, [cb]);
const wrapper = useCallback((...args) => {
ref.current(...args);
}, []);
return wrapper;
}; |
Please Keep the name. It's far less abstract than the other proposals, which will help a lot of non-native speakers. |
@franciscop there's an example of the internal implementation of this hook in the proposal: https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md#internal-implementation |
What happens when the event is asynchronous? This is a big usecase IMHO and a big pain point in current React's useEffect where you cannot do const onClick = useEvent(async e => {
...
}); I think for a Hook that is going to be used in a highly-async environment, it'd be nice to have some Disclaimer: I've written part of |
Sorry to continue with the bikeshedding on the name of the hook, but I think my perspective is slightly different from what has been shared already. I think the concept of "event" aligns well with what these functions are intended for. Given they're not meant to be used during render (they can only be used when the tree has been committed) the only moment when they can actually execute is during an event (a user interaction event, a network event, a timer event, etc.). My only concern is about calling it just useEvent('click', () => {
// do something
}); Which isn't what it does. In order to avoid this confusion, I think we should use something like |
|
@franciscop what kind of help are you thinking about here? What do you think React should do differently when passing an async function instead of a sync one, considering React just returns a stable version of the function but doesn't really call that function itself? |
First and foremost, this solves a huge pain point for the community. I love it so much. The over proliferation of In terms of naming: I think you could call it Perhaps clarify guidance around async functions, e.g. if you are using an async action it might be better to treat it as a Promise rather than using |
I'm thinking to at least allow the function to be async, since if it's async in useEffect What I was thinking as active help is that if this function (which doesn't seem to be the case from the RFC, but the name is slightly confusing) was supposed to be to manage only events like Edit: I should re-read the docs more often, React keeps getting better in small ways (not needing |
I'd also argue strongly against |
Hey folks, thanks to everyone for the discussion! We've implemented a prototype in facebook/react#25229 so you can play with it in the In particular, we're worried that this proposal creates a strong incentive to wrap every function that currently uses For rendering optimizations, we are researching and developing an auto-memoizing compiler. It's not yet clear if the optimization described in this RFC would still be impactful when the compiler is on, so we will be revisiting this question as a part of that ongoing research. There are also other alternative ways to optimize this pattern that we might want to consider. For now, we would like to shelve the current RFC, with a plan to publish a different, more scoped down RFC that supersedes this one. Since the new RFC will be different in scope, we might also give that Hook a different name. tl;dr
|
Thank you, @gaearon and Team, for putting this out and moderating the sometimes contentious debates. I appreciate the goal behind this RFC and look forward to its replacement. |
To clarify, the current plan is for the next RFC to have the same API, but with slightly different usage recommendations, slightly different semantics, and possibly a different name. I’ve seen some posts going around saying this RFC is “dead”, and that seems to give an impression that we’re abandoning this entire approach. In reality, we want to split this proposal and add a few extra constraints to it. |
In case this datapoint is helpful for the other RFCs, we implemented a slightly modified version of the proposed Implementation:
Learnings:
|
IMHO the question useCallback vs useEvent is not about the return type. As @gaearon says:
I second that. It's not about the implementation. It's about the consumer. |
This commit adds a new hook `useEvent` per the RFC [here](reactjs/rfcs#220), gated as experimental. Co-authored-by: Rick Hanlon <rickhanlonii@gmail.com> Co-authored-by: Rick Hanlon <rickhanlonii@fb.com> Co-authored-by: Lauren Tan <poteto@users.noreply.github.com>
what about |
@mrcljx your findings are really interesting. It's great to see that even if the RFC isn't finalized, the core idea behind this is beyond a POC and viable with the correct usage.
Do you mind sharing how you achieved this? We're curious to try this out but couldn't quite figure out the change necessary for this to work. The Did you just add an additional scenario around the following line with code similar to this and re-build the extension? if (name === 'useEvent' && id.type === 'Identifier') { // "useEvent" being the name of the custom hook
return true
} Thanks! |
@sgarcia-dev Yes, this is our patch (we also patched in }
const id = def.node.id;
const { name } = callee;
- if (name === "useRef" && id.type === "Identifier") {
+ if (name === "useLatest" && id.type === "Identifier") {
+ // useLatest() return value is stable.
+ return true;
+ } else if (name === "useEvent" && id.type === "Identifier") {
+ // useEvent() return value is stable.
+ return true;
+ } else if (name === "useRef" && id.type === "Identifier") {
// useRef() return value is stable.
return true;
} else if (name === "useState" || name === "useReducer") { |
The `useEvent` function was being called during rendering, which throws an error when React's Strict Mode is enabled. This type of usage is incorrect because it makes the rendering result non-determinisitic. See reactjs/rfcs#220 (comment)
…146352) ## Summary This PR includes a number of updates to Unified Histogram to prepare for sharing with solutions, as well as updating its usage in Discover: Unified Histogram: - Adds a `disableAutoFetching` prop, similar to Unified Field List, to disable automatic Unified Histogram refetching based on prop changes. - Accepts an `input$` observable prop that can be used to control manual refetches. - Removes `refetchId` used internally and replaces it with an observables based approach to simplify refetch logic. - Introduces a `use_stable_callback` utility hook to create callback functions with stable identities which simplifies `useCallback` logic — should be replaced with `useEvent` or whatever the React team comes up with to solve this specific issue when available: reactjs/rfcs#220. - Eliminates debouncing logic in Unified Histogram since it was hacky — manual refetching should be used instead if debouncing is needed. - Accepts `query`, `filters`, and `timeRange` props to remove dependencies on specific services. - Updates `use_request_params` to export `getTimeRange` and `updateTimeRange` functions for easier absolute time range handling. - Exposes some Lens embeddable props to allow further customizing Unified Histogram behaviour. Discover: - Exports a `fetch$` observable from `use_saved_search` to allow listening for when data fetches should be triggered. - Uses new manual refetching support in Unified Histogram to simplify refetching logic. - Passes `query`, `filters`, and `timeRange` props to Unified Histogram. ### Checklist - [ ] ~Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)~ - [ ] ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))~ - [ ] ~Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))~ - [ ] ~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~ - [ ] ~This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))~ - [ ] ~This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)~ ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
…lastic#146352) ## Summary This PR includes a number of updates to Unified Histogram to prepare for sharing with solutions, as well as updating its usage in Discover: Unified Histogram: - Adds a `disableAutoFetching` prop, similar to Unified Field List, to disable automatic Unified Histogram refetching based on prop changes. - Accepts an `input$` observable prop that can be used to control manual refetches. - Removes `refetchId` used internally and replaces it with an observables based approach to simplify refetch logic. - Introduces a `use_stable_callback` utility hook to create callback functions with stable identities which simplifies `useCallback` logic — should be replaced with `useEvent` or whatever the React team comes up with to solve this specific issue when available: reactjs/rfcs#220. - Eliminates debouncing logic in Unified Histogram since it was hacky — manual refetching should be used instead if debouncing is needed. - Accepts `query`, `filters`, and `timeRange` props to remove dependencies on specific services. - Updates `use_request_params` to export `getTimeRange` and `updateTimeRange` functions for easier absolute time range handling. - Exposes some Lens embeddable props to allow further customizing Unified Histogram behaviour. Discover: - Exports a `fetch$` observable from `use_saved_search` to allow listening for when data fetches should be triggered. - Uses new manual refetching support in Unified Histogram to simplify refetching logic. - Passes `query`, `filters`, and `timeRange` props to Unified Histogram. ### Checklist - [ ] ~Any text added follows [EUI's writing guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses sentence case text and includes [i18n support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)~ - [ ] ~[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html) was added for features that require explanation or tutorials~ - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [ ] ~Any UI touched in this PR is usable by keyboard only (learn more about [keyboard accessibility](https://webaim.org/techniques/keyboard/))~ - [ ] ~Any UI touched in this PR does not create any new axe failures (run axe in browser: [FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/), [Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))~ - [ ] ~If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the [docker list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)~ - [ ] ~This renders correctly on smaller devices using a responsive layout. (You can test this [in your browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))~ - [ ] ~This was checked for [cross-browser compatibility](https://www.elastic.co/support/matrix#matrix_browsers)~ ### For maintainers - [ ] This was checked for breaking API changes and was [labeled appropriately](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
The `useEvent` function was being called during rendering, which throws an error when React's Strict Mode is enabled. This type of usage is incorrect because it makes the rendering result non-determinisitic. See reactjs/rfcs#220 (comment)
Every additional API increases the mental burden on users Now, users have to worry about when to use The two functions are quite similar, why not upgrade I suggest considering enhancing the functionality of existing APIs while maintaining compatibility and asymptotic behavior. |
has there been new RFC created? Can it be linked to this one? A lot of materials - quite outdated, yes - refer this current RFC. And without follow-up link it's just a dead-end |
Where can I find further discussion? Any updates here? and what's the progress? |
In this RFC, we propose a
useEvent
Hook. It lets you define event handlers that can read the latest props/state but have always stable function identity. Event handlers defined withuseEvent
don't break memoization and don't retrigger effects.View formatted RFC
FAQ:
useCallback
?Update (Sep 2022): we're planning to revise this proposal (and possibly rename it)