Skip to content

RFC: Extends memo function to support mutable event handlers. #240

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
141 changes: 141 additions & 0 deletions text/0000-extends-memo-for-event-handler.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
- Start Date: 2023-02-08
- RFC PR: (leave this empty)
- React Issue: (leave this empty)

# Summary

Extends `memo` function to support mutable event handlers.

# Basic example

You need not wrap any event handler by `useCallback` or `useEvent`.

```js
import OriginalSendButton from 'some-where';

let SendButton = OriginalSendButton;

// When you want to optimize `SendButton`, just memoization like:
SendButton = memo(SendButton, {events: true});

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

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

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

# Motivation

Make `memo` work for mutable event handlers.

It is similar with `useEvent` rfc [first motivation](https://github.com/reactjs/rfcs/blob/useevent/text/0000-useevent.md#reading-stateprops-in-event-handlers-breaks-optimizations).

Before `memo` will break by mutable event handlers.

So a lot of people wrap every event handler with `useCallback` or `useEvent`.
The wrapper is annoying, and most of the time it just makes performance worse.


# Detailed design

## Internal implementation

Internally, `memo` function will approximately work like this:

```jsx
// (!) Approximate behavior
import {memo as reactMemo, forwardRef, useRef} from 'react';

export default function memo(WrappedComponent, options) {
if (!options || typeof options === 'function') {
return reactMemo(WrappedComponent, options);
}

const MemoComponent = reactMemo(Wrapped, options.propsAreEqual);

const {events} = options;

if (!events) return MemoComponent;

// events is an array or true
const Wrapped = forwardRef((props, ref) => {
const memoEventsRef = useRef({});
const eventNames = Array.isArray(events)
? events
: Object.keys(props).filter((k) => /^on[A-Z]/.test(k));

const memoEvents = eventNames.reduce(
(me, n) => ({
...me,
[n]: fixEventHandler(memoEventsRef.current[e], props[e])
}),
{}
);
memoEventsRef.current = memoEvents;
return <MemoComponent ref={ref} {...props} {...memoEvents} />;
});

Wrapped.displayName = `FixedEvent(${getDisplayName(MemoComponent)})`;

return Wrapped;
}

const wm = new WeakMap();

function fixEventHandler(fixed, handler) {
if (typeof handler !== 'function' || wm.has(handler)) return handler;
if (!fixed) {
fixed = (...params) => {
const h = wm.get(fixed);
return h(...params);
};
}

wm.set(fixed, handler);
return fixed;
}

function getDisplayName(WrappedComponent) {
return WrappedComponent.displayName || WrappedComponent.name || 'Component';
}

```

IMHO, the essence of the problem is that the event handler should not be regarded as an ordinary callback.
So we need to add a proxy layer to the event as above.

# Drawbacks

- If the event props does not follow the naming convention, it will cause unnecessary wrapping.
- There will be some performance overhead for detecting event props every render.
- In order to be compatible with the original behavior, the default parameter settings may not be reasonable.

# Alternatives

`useCallback`, `useEvent` and some other wrapper hooks put too much mental load on the user.
User may don not know when to use or not. This can easily lead to premature optimization and reverse optimization.


# Adoption strategy

Release it in a minor.
Use the linter to advise users to remove `useCallback` wrappers for functions starting with on* or handle*.
Write new documentation teaching common patterns.

Warn for `memo` without `events` option.

# How we teach this

In the `memo` documentation, recommend to use this feature.

In the `useCallback` documentation, emphasis callback and event handler are two different concepts.

# Unresolved questions

- How to teach and warn people not use event handler as callback on render time?
- How to add the similar behavior to `PureComponent` and `shouldComponentUpdate`.