-
Notifications
You must be signed in to change notification settings - Fork 46.9k
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
useCallback() invalidates too often in practice #14099
Comments
@gaearon thank you for your answer in reactjs/rfcs#83. I've look at sources of |
In concurrent mode (not yet released), it would "remember" the last rendered version, which isn't great if we render different work-in-progress priorities. So it's not "async safe". |
Would be nice if second argument of function useCallback(cb, deps) => {
lastDeps = deps; // save current deps and cb deep in somewhere
lastCb = cb;
if (!cached) {
cached = (...args) => lastCb(...lastDeps)(...args); // memoize that forevere
}
return cached; // never invalidates
}
const myCallback = useCallback(
(state, props) => (a, b) => a + b + state + props,
[state, props]
);
myCallback(1, 2) |
const useCallback = (fn, args) => {
const callback = useMemo(() => {
if (__DEV__) {
if (fn.length !== args.length) warning(...);
}
const callback = () => fn(...callback.args);
return callback;
});
useEffect(() => callback.args = args, [args]);
return callback;
} Drawbacks: It's easy to forget the arguments list, which would result in hard to find bugs. In dev mode it would make sense to check fn.length for the correct length. It's still possible to forget arguments in the dependencies array, but this applies to other hooks too. |
Yes, that's the approach from reactjs/rfcs#83 and https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback. We don't want it to be default because it's easier to introduce bugs in concurrent mode this way. |
@sokra An alternate would be: function useEventCallback(fn) {
let ref = useRef();
useLayoutEffect(() => {
ref.current = fn;
});
return useCallback(() => (0, ref.current)(), []);
} This doesn't require the args like yours has. But again, you can't call this in the render phase and the use of mutation is dicey for concurrent. |
@sophiebits That's clever and would have none of the problems with args, etc. It even doesn't require a dependencies list. One nitpick: |
@sokra With this you will not be able to access to state and props updates inside a callback. const [state, setState] = useState(0);
const handleClick = useCallback((event) => {
console.log(state); // always 0
setState(s => s + 1);
});
return <button onClick={handleClick} /> So dependencies are required. function useCallback(fn, deps) {
const fnRef = useRef(fn);
const depsRef = useRef(deps);
useLayoutEffect(() => {
fnRef.current = fn;
depsRef.current = deps;
});
return useCallback((...args) => (0, ref.current)(...depsRef.current)(...args), []);
} cons handleClick = useCallback(
(state) => event => console.log(state), // up-to-date
[state]
); |
I think with @sophiebits' approach this will work. The latest function is always copied into the ref and only a trampoline function is returned. This will make sure that the latest function is called, which has the latest state in context. |
I recently made a duplicate issue and was asked to check this one. What I proposed there was very similar to @sophiebits' approach, but still looks a bit simpler to me: function useStatic(cb) {
const callback = useRef(cb)
callback.current = cb
const mem = useRef((...args) => callback.current(...args))
return mem.current
// We could, of course still, use the useCallback hook instead of a second reference.
// return useCallback((...args) => callback.current(...args), [])
// Although I think that the one above is better since it avoids the need to compare anything at all.
} This way it is guaranteed to update where the hook is called since it does not directly use any side effect and instead it only updates a reference. It seems to me that it should be callable during the render phase and should not be dicey with concurrent mode (unless references don't meet these two conditions). Wouldn't this approach be a little better or am I missing something? |
@muqg In concurrent mode, last render doesn't necessarily mean "latest committed state". So a low-priority render with new props or state would overwrite a reference used by current event handler. |
If I understand the problem correctly... What if useCallback will return a special callable object with two different states of an our callback function? The first is a function Callback(cb) {
function callback(...args) {
return callback.current(...args);
}
callback.commited = cb;
callback.current = cb;
callback.SPECIAL_MARKER_FOR_REACT = true;
return callback;
}
function useCallback(cb) {
const callback = useMemo(() => new Callback(cb), []);
callback.current = cb;
useLayoutEffect(() => {
callback.commited = cb;
});
return callback;
} function Component(counter) {
const handler = useCallback(() => {
console.log(counter);
});
handler(); // call to handler.current
// pass handler.commited to the dom
return <button onClick={handler} />
} |
I don't think there is a point in saving the "current", if you want to call it during rendering, just save it in advance out of the hook:
I personally don't see any benefits of the current |
Concurrent mode can produce two different representations of a component (the first one is that commited to the dom and the second one is that in memory). This representations should behaves accordingly with their props and state.
@muqg proposal mutate the callback on each render, so the The point of my proposal in the passing a separated callback reference, that will changes in commit phase, to the dom, while the in-memory (not commited) representation of a component can use the latest version of that callback. const handler = () => {/* do something*/};
const callback = useCallback(handler); In this case, you wont pass down the |
Hi. |
How/why is it that variables are dereferenced within Whether or not the effect is called again based on changes to state/reducer/etc ( This behavior seems unexpected and having to leverage "escape hatches" just feels broken to me. |
I have a problem with converting this kind of thing to use functional components and the useCallback hook...
The PickField, TextField and DateField components can be implemented with React.PureComponent or React.memo(...). Basically they just display an input field and a label - they have their own onChange handler which calls the onChangeField prop passed in. They only redraw if their specific value changes onChangeField as above works just fine - but if I try this using a functional component for TestForm and useCallback for onChangeField I just can't get it to 'not' redraw everything on a single field change |
@Bazzer588 Are you using React.memo on your functional component? What do your attempts using hooks/functional look like? Your problem may or may not be related to this issue; it's hard to tell without seeing your code. |
Here's the full code - just drop a Using a React.Component, only the field being editted does a full render
Using Hooks, all the child elements are re-rendered every time - presumably as onChangeField changes every time the state data changes. Is there some way I can implement onChangeField so it behaves like the above example?
This is my
|
@Bazzer588 I think its due to the object value kept in state under the variable name of I've had a similar issue like you and noticed this as being its cause. I have no idea why the object in state does not keep its reference when it has not been explicitly set to a new object. |
Yes my problem is that the in first example (using React.Component) I can create a onChangeField callback which is bound to this, and never changes during renders Using the new hook methods I can't seem to replicate the way the existing functionality works, On the project I'm working on we often do this to have a hierachy of components and state:
So it passes props down the tree (ie value {}) |
Use |
@Bazzer588 The data var your passing to the second parameter of useCallback is going to invalidate every time. useCallback doesn't do a deep comparison. You need to pass in a flat array for it to properly memoize. |
@nikparo
I disagree.
Secondary deps array does something:
https://github.com/gfox1984/granular-hooks/blob/8fbb19cbb949e5cd7f15827c4a6a6a60c2465788/src/hooks/useGranularHook.ts#L17
It is clearly being included in what's passed to the callback, along with the primary deps.
It's only running the equivalency check on the primary deps.
|
@Hypnosphi So basically, if I'm not going to use concurrent mode, this is fine then? |
@BirdTho They are being passed to the hook* (not callback), sure, but they are only updated when the primary deps are updated. There is no magic connection between the values in the closure and the values in the dependency array. In other words, the callback closure remains stale until the primary deps change. But this is getting off topic, so if you don't believe me I suggest you simply test it yourself with and without the so called secondary deps. |
Seems like it's useless with onChange events for input: type InputProps = {
name: string;
value: string;
setValue: (value: string) => void;
};
const Input: React.FC<InputPropsA> = ({ name, value, setValue }) => {
const onChangeHandler = useEvent((e: React.ChangeEvent<HTMLInputElement>) => {
setValue(e.target.value);
});
return <input name={name} onChange={onChangeHandler} value={value} />;
};
const Form = () => {
const [firstname, setFirstname] = useState('');
const [lastname, setLastname] = useState('');
const [middlename, setMiddlename] = useState('');
return (
<form>
<Input name="firstname" setValue={setFirstname} value={firstname} />
<Input name="lastname" setValue={setLastname} value={lastname} />
<Input name="middlename" setValue={setMiddlename} value={middlename} />
</form>
);
}; Even if you put every setState into useCallback - it's not working for me right now with this hook If I'm doing something wrong, pelase guide me. |
The main idea is to memoize a callback, which is passed to another component. In your example you're trying to memoize a callback, which is passed into So, in your example you could memoize set*** callback. But in your case they are memoized already) But, I have a fix for your example: type InputProps = {
name: string;
value: string;
setValue: (value: string) => void;
onClick: (value: string) => void;
};
// You have to use memo() here, to make memoization work
const Input = memo<InputPropsA>(({ name, value, setValue, onClick }) => {
// You do not need any memoization here for onChange or onClick handler
return <input name={name} onChange={(e) => e.taget.value} value={value} onClick={() => onClick(value)} />;
});
const Form = () => {
const [firstname, setFirstname] = useState('');
const [lastname, setLastname] = useState('');
const [middlename, setMiddlename] = useState('');
// And this is a correct memoization, cause onInputClick is passed into Input
const onInputClick = useEvent((value: string) => {
console.log(`value is: ${value}`);
});
return (
<form>
<Input name="firstname" setValue={setFirstname} value={firstname} onClick={onInputClick} />
<Input name="lastname" setValue={setLastname} value={lastname} onClick={onInputClick} />
<Input name="middlename" setValue={setMiddlename} value={middlename} onClick={onInputClick} />
</form>
);
}; Right now, |
to be honest, I'm not sure why the implementation of the useEvent has to be so complicated, why not just this? function useEvent(callback) {
const callbackRef = useRef(callback)
callbackRef.current = callback
return useCallback((...args) => {
return callbackRef.current(...args)
}, [])
} below is the typescript version function useEvent<ArgumentsType extends unknown[], ReturnType>(
callback: (...args: ArgumentsType) => ReturnType,
) {
const callbackRef = useRef(callback)
callbackRef.current = callback
return useCallback((...args: ArgumentsType): ReturnType => {
return callbackRef.current(...args)
}, [])
} with this implementation, there is no such thing as when will the current is switched, because it will be switched every single time, and it can be used in rendering as well |
@kambing86 this will not work properly in "concurrent mode", see #14099 (comment) |
function getStateAsync(setter) {
return new Promise(resolve => {
setter(prevValue => {
resolve(prevValue);
return prevValue; // The value has not changed.
});
})
}
function Chat() {
const [text, setText] = useState('');
const onClick = useCallback(async () => {
sendMessage(await getStateAsync(setText));
}, []); // There is no dependency array.
return <SendButton onClick={onClick} />;
} This gist is written in typescript and allows you to get several states at once. |
While it's not fully available, what could we use to achieve the closest behavior? https://github.com/vadzim/react-use-handler, https://github.com/gfox1984/granular-hooks, or some gist above? I'm using React 16 |
@sophiebits wouldn't function useEventCallback(fn) {
const ref = useRef(fn);
useImperativeHandle(ref, () => fn);
return useCallback((...args) => ref.current?.(...args), []);
} If thats not the case, can you explain why? - Just curious whether I'm understanding everything properly. |
I think we can save some overhead by writing it this way (avoiding what seems to be an unnecessary export const useEvent = (fn) => {
const ref = useRef([fn, (...args) => ref[0](...args)]).current;
useLayoutEffect(() => {
ref[0] = fn;
});
return ref[1];
} |
I bumped into this thread while already having a solution and trying to understand if I'm reinventing the wheel :) Looks like I'm not, as my solution to the problem is different than existing options: export default function useConstCallback<T extends CallableFunction>(func: T, deps?: React.DependencyList): T {
const ref = useRef<T>(func)
useMemo(() => {
ref.current = func
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps)
return useCallback((...params) => ref.current(...params), []) as unknown as T
} The main difference compared to Considering At the first glance, it may have the same issues with |
Stuff like this is why Vue or Angular are still alive.
…On Sat, Jan 28, 2023, 7:57 AM ZZYZX ***@***.***> wrote:
I bumped into this thread while already having a solution and trying to
understand if I'm reinventing the wheel :)
Looks like I'm not, as my solution to the problem is different than
existing options:
export default function useConstCallback<T extends CallableFunction>(func: T, deps?: React.DependencyList): T {
const ref = useRef<T>(func)
useMemo(() => {
ref.current = func
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps)
return useCallback((...params) => ref.current(...params), []) as unknown as T}
The main difference compared to useEvent above is that I'm abusing
useMemo() (which runs instantly during hook call) instead of using
useLayoutEffect() or useEffect() which calls only after the render;
Considering useCallback(x, deps) is functionally the same as useMemo(()
=> x, deps) (see implementation in React:
https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberHooks.js#L2242),
this works exactly like useCallback() would, with the exception of having
a constant reference.
—
Reply to this email directly, view it on GitHub
<#14099 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ACR376PUOJWLDN3ZRZEVPF3WUUJR7ANCNFSM4GB2QADA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Given that the 'useEvent' hook has gone the way of the doodoo what are the plans to solve this issue? I was a big fan of the 'useEvent' hook - and end up having to copy my own implementation in every other project (something like use-event-callback or the one @HansBrende mentioned in that issue). To bring this issue to a final close why not encourage a simply "hook recipe" on the documentation? Something like:
I understand that the team is currently investigating other options for the 'rendering optimizations are cumbersome to type' problem- but us every day developers don't have access to these tools. Indeed, should they be released we might not be able to use them because we're stuck on an old version of React for some reason or another. |
Had a quite interaction heavy app, so had to optimize rendering quite heavily. So leaned into function App() {
let [a, setA] = useState()
let [b, setB] = useState()
useEffect(() => {
setA(a => {
setB(b => {
callApi(a,b)
return b
})
return a
})
}, [])
// which fundamentally can be changed to either adding custom hook,
// or extending React's useEffect/useMemo/useCallback via RFC
useStateEffect((a,b) => {
callApi(a,b)
}, [], [setA, setB])
} Which, I think, precisely solves all the problems: optimize useCallback, premature "useEvent all the things". But requires passing setStates instead of state itself as props. Also, not sure about this concern in another issue, maybe I'm missing something:
|
Is this the correct code? To me it reads like- |
@vzaidman you're reading correctly. But, I have to concede, the code is quite abstracted (i.e. you can think of useCallback/useMemo instead of useEffect) UPD: After a bit of time, I think we don't really need setters: function ComponentName({a}) {
const [b, setB] = useState(0);
const cb = useMemo((a,b) =>
(event) => {callbackDeps1(event); callbackDeps2(a,b)},
[callbackDeps1, callbackDeps2],
[a,b]
);
return <Button onClick={cb} />
} |
Can you make this code more complex, its too simple |
Looks really weird. Why didn't you just use |
Perhaps a stupid question. What is the |
@tibbe const f = ref.current;
f() |
This is related to #14092, #14066, reactjs/rfcs#83, and some other issues.
The problem is that we often want to avoid invalidating a callback (e.g. to preserve shallow equality below or to avoid re-subscriptions in the effects). But if it depends on props or state, it's likely it'll invalidate too often. See #14092 (comment) for current workarounds.
useReducer
doesn't suffer from this because the reducer is evaluated directly in the render phase. @sebmarkbage had an idea about givinguseCallback
similar semantics but it'll likely require complex implementation work. Seems like we'd have to do something like this though.I'm filing this just to acknowledge the issue exists, and to track further work on this.
The text was updated successfully, but these errors were encountered: