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

RFC: useEvent #220

Closed
wants to merge 2 commits into from
Closed

RFC: useEvent #220

wants to merge 2 commits into from

Conversation

gaearon
Copy link
Member

@gaearon gaearon commented May 4, 2022

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 with useEvent don't break memoization and don't retrigger effects.

View formatted RFC

FAQ:


Update (Sep 2022): we're planning to revise this proposal (and possibly rename it)

@windmaomao
Copy link

Finally something easy to think of is implemented as it seems. 👍🏻

@ds300
Copy link

ds300 commented May 4, 2022

Great idea ❤️

My main suggestion would be a different name. useEvent is not a very precise way to convey what this hook does. I've seen it implemented in other codebases as useStableCallback, which I slightly prefer, although I see that you've already dismissed it as being too verbose in the RFC.

@Merri
Copy link

Merri commented May 4, 2022

Or maybe just name this useCallback and make it work for both cases, but deprecate the passing of dependencies and remove some version later.

Unless someone can figure out any valid use case for the old useCallback functionality with deps.

@windmaomao
Copy link

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 useGlobal or useStatic.

@panta82
Copy link

panta82 commented May 4, 2022

I've been using this all over the place. I call it useLatestClosure. It's definitely the missing piece of React.

I don't like useEvent naming. IMO using these as events will be a very minor part of usage pattern, just an optimization in the largest codebases. Mostly it will be used when you need to pass a function somewhere where you don't want to trigger re-render.

Give the useEvent behavior to useCallback. We don’t want to do this because they have sufficiently different semantics.

I think this would be a better change. I almost always want to use useLatestClosure (or useEvent) and almost never useCallback with an argument list.

As Merri said, useCallback should just adopt this functionality when called without deps. And deps usage should be an advanced optimization use case.

@nikparo
Copy link

nikparo commented May 4, 2022

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?

Same proposal, but allow calling event handlers during rendering. We think this creates too many footguns.

If calling event handlers during rendering was allowed, then would the event handler be up to date or not? I.e. would isCurrent in the example below always be true? If that is a possibility, then I think it warrants serious consideration.

  const [value, setValue] = useState(0);
  const getCurrentValue = useEvent(() => value);

  // always true or sometimes false?
  const isCurrent = value === getCurrentValue();

@kocyigityunus
Copy link

kocyigityunus commented May 4, 2022

I didn't like the useEvent naming too. maybe split it into two different hooks.

  1. useChange => with dependencies, since it re-runs when one of the dependencies changes.
  2. useFunction or useFunctionRef => without dependencies. since it's basically useRef for a function.

@franciscop
Copy link

franciscop commented May 4, 2022

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;
};

Nevertheless, text inside useEvent will reflect its latest value

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;
};

@alvarlagerlof
Copy link

Please Keep the name. It's far less abstract than the other proposals, which will help a lot of non-native speakers.

@rubennorte
Copy link

@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

@franciscop
Copy link

franciscop commented May 4, 2022

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 useEffect(async () => {...});. I see little mention of this very typical usecase (only "events should usually not be asynchronous"), what happens here? Is this valid?

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 async help in-place. Like, at the very least allow the callback to be async by design.

Disclaimer: I've written part of use-async-effect and all of use-async 😬

@rubennorte
Copy link

rubennorte commented May 4, 2022

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 because the name might suggest this is to subscribe to events, like:

useEvent('click', () => {
  // do something
});

Which isn't what it does.

In order to avoid this confusion, I think we should use something like useEventHandler or useEventCallback.

@so1ua
Copy link

so1ua commented May 4, 2022

useFunction is better :) Especially that as I understand useCallback will almost not be used later

@rubennorte
Copy link

rubennorte commented May 4, 2022

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 useEffect(async () => {...});. I see little mention of this very typical usecase (only "events should usually not be asynchronous"), what happens here? Is this valid?

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 async help in-place. Like, at the very least allow the callback to be async by design.

Disclaimer: I've written part of use-async-effect and all of use-async 😬

@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?

@ScottAwesome
Copy link

First and foremost, this solves a huge pain point for the community. I love it so much. The over proliferation of useCallback and associated bugs are solved pretty concisely in this addition.

In terms of naming: I think you could call it useEventHandler. I see the purpose of useCallback being useful for things where you need data invalidation to change the function signature, like when interacting with results from an API. I does become more of an edge case tool but that's kind of the point? Arguably, one could just use useMemo for complex cases too, but the simplicity of useCallback for non event handling cases is nice.

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 async and await?

@franciscop
Copy link

franciscop commented May 4, 2022

I'm thinking to at least allow the function to be async, since if it's async in useEffect even when the return value is not used officially it'll give you a strong warning an error. Like, as far as it doesn't get in the way of that I'd be happy already.

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 onClick it could also help by calling event.persist() by default. Though I'm just reading that it seems that event.persist() is useless since React 17 and I didn't know that, so there might be nothing else to do here!

Edit: I should re-read the docs more often, React keeps getting better in small ways (not needing e.persist()) and I didn't even notice.

@sompylasar
Copy link

I'd also argue strongly against useEvent name because an "event" is something that happens, but what we're making with this hook is a callback function which is supposed to handle/process the happened event. Suggest more specific useEventCallback or useEventHandler instead, even though it's longer to type. I would avoid going as abstract as useLatestClosure or useFunction.

@gaearon
Copy link
Member Author

gaearon commented Sep 27, 2022

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 @experimental releases. However, we've also realized that we don't feel confident in coupling the two different motivations (rendering optimization and "fixing" Effects) under this single proposal.

In particular, we're worried that this proposal creates a strong incentive to wrap every function that currently uses useCallback into useEvent. However, that wouldn't lead to right semantics where the function identity is important. Since useEvent "erases" reactivity of the values flowing into it, we think there need to be additional limitations on where it makes sense to do the wrapping. In most cases, only the final consuming side (e.g. the Effect from which the event function is called) can be "sure" whether it "cares" about the reactivity of the function value. So using it widely seems premature.

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

  • We're still committed to solving both problems in Motivation, but maybe not at the same time.
  • We'll submit a more focused RFC to deal with the second motivation (preventing re-triggering of useEffect).
  • We expect that updated RFC will be similar to this one but have slight differences in usage and possibly naming.

@gaearon gaearon closed this Sep 27, 2022
@landisdesign
Copy link

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.

@gaearon
Copy link
Member Author

gaearon commented Sep 28, 2022

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.

@mrcljx
Copy link

mrcljx commented Sep 29, 2022

In particular, we're worried that this proposal creates a strong incentive to wrap every function that currently uses useCallback into useEvent. However, that wouldn't lead to right semantics where the function identity is important.

In case this datapoint is helpful for the other RFCs, we implemented a slightly modified version of the proposed useEffect to help with the semantics.

Implementation:

  1. The implementation is the user-space implementation.
    1. Future: We'll probably switch to the experimental implementation.
  2. Our signature requires that the callback's return type must be (async) void.
    function useEvent<T extends (...args: any[]) => void | Promise<void>>(callback: T): T;
    
    1. A useEventWithResult variant exists that allows non-void results. It was deliberately picked to be "longer" to write and comes with a big "be sure that this isn't a case for useCallback" warning.
    2. Alternatives considered: We originally planned to write an ESLint rule to ensure that the variables that useEvent results are stored in are named {on,handle,set}XYZ, however solving this through seemed to better fit the semantics.
    3. Future: we might be a bit less strict about Promise and actually allow void | Promise<unknown>.
  3. We forked eslint-plugin-react-hooks to treat the return values of these new hooks as stable when it comes to exhaustive-deps.
  4. We then went through our codebase and replaced each const {on,handle,set}XYZ = useCallback with useEvent. A few cases were pointed out where the callback actually returned a value and needed to be replaced useEventWithResult.

Learnings:

  1. Our codebase now roughly has a 1:4 share of useCallback to useEvent (120 vs 430).
    1. It means in those 430 cases we were able to remove the deps array.
    2. Because of the void-constraint we are very sure that this was correct semantically.
  2. The term useEvent is still a bit odd.
    1. Intent is a bit unclear.
    2. Conflict with the concept of "events" (we had to renamed a different hook to useDOMEvent).
    3. Future: If React would not implement this RFC, we'd consider switching to useHandler (or useEventHandler) and useHandlerWithResult as a nod to the class-based component convention of naming event handlers (fun fact: in C# an EventHandler is required to be void as well).

@jampy
Copy link

jampy commented Oct 3, 2022

Our signature requires that the callback's return type must be (async) void.

IMHO the question useCallback vs useEvent is not about the return type.

As @gaearon says:

In most cases, only the final consuming side (e.g. the Effect from which the event function is called) can be "sure" whether it "cares" about the reactivity of the function value.

I second that. It's not about the implementation. It's about the consumer.

rickhanlonii added a commit to facebook/react that referenced this pull request Oct 5, 2022
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>
@yolio2003
Copy link

what about useNonReactiveHandler

@sgarcia-dev
Copy link

sgarcia-dev commented Oct 21, 2022

@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.

3. We forked eslint-plugin-react-hooks to treat the return values of these new hooks as stable when it comes to exhaustive-deps.

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 isStableKnownHookValue appears to be the function in charge of marking dependencies as safe to ignore.
https://github.com/facebook/react/blob/c635807875630e7057777e898372eed43e3b0a24/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L164

Did you just add an additional scenario around the following line with code similar to this and re-build the extension?
https://github.com/facebook/react/blob/c635807875630e7057777e898372eed43e3b0a24/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js#L226-L228

if (name === 'useEvent' && id.type === 'Identifier') { // "useEvent" being the name of the custom hook
  return true
}

Thanks!

@mrcljx
Copy link

mrcljx commented Oct 21, 2022

@sgarcia-dev Yes, this is our patch (we also patched in useLatest which returns a Ref like useRef).

         }
         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") {

pawelgrimm added a commit to Doist/typist that referenced this pull request Jan 13, 2023
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)
davismcphee added a commit to elastic/kibana that referenced this pull request Jan 18, 2023
…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>
wayneseymour pushed a commit to wayneseymour/kibana that referenced this pull request Jan 19, 2023
…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>
pawelgrimm added a commit to Doist/typist that referenced this pull request Feb 22, 2023
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)
@zhangfisher
Copy link

Every additional API increases the mental burden on users

Now, users have to worry about when to use useCallback and when to use useEvent.

The two functions are quite similar, why not upgrade useCallback to have the useEvent function by default or through option switches?

I suggest considering enhancing the functionality of existing APIs while maintaining compatibility and asymptotic behavior.

@skyboyer
Copy link

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

@dante01yoon
Copy link

Where can I find further discussion? Any updates here? and what's the progress?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.