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

Memoize mutation callbacks #8526

Merged
merged 2 commits into from
Jan 11, 2023
Merged

Memoize mutation callbacks #8526

merged 2 commits into from
Jan 11, 2023

Conversation

rkfg
Copy link
Contributor

@rkfg rkfg commented Dec 20, 2022

Fixes #8518.

@vercel
Copy link

vercel bot commented Dec 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-admin 🔄 Building (Inspect) Dec 20, 2022 at 0:41AM (UTC)

Copy link
Contributor

@slax57 slax57 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It seems good to me, but I'd feel safer having @fzaninotto review that one too 😁

@slax57 slax57 requested a review from fzaninotto December 20, 2022 14:45
@slax57 slax57 added the RFR Ready For Review label Dec 20, 2022
@fzaninotto
Copy link
Member

fzaninotto commented Dec 20, 2022

This won't work - because passing an empty array as dependency means the callback will never be updated. The useLayoutEffect should always fire, so it should receive no second argument. I led you on a false track - useEventCallback cannot do the trick. You'll need to create a useEvent shim:

import * as React from 'react';
import { useCallback } from 'react';

// allow the hook to work in SSR
const useLayoutEffect =
    typeof window !== 'undefined' ? React.useLayoutEffect : React.useEffect;

/**
 * Alternative to useCallback that doesn't update the callback when dependencies change
 *
 * @see https://reactjs.org/docs/hooks-faq.html#how-to-read-an-often-changing-value-from-usecallback
 * @see https://github.com/facebook/react/issues/14099#issuecomment-440013892
 */
export const useEvent = <Args extends unknown[], Return>(
    fn: (...args: Args) => Return,
): ((...args: Args) => Return) => {
    const ref = React.useRef<(...args: Args) => Return>(() => {
        throw new Error('Cannot call an event handler while rendering.');
    });

    useLayoutEffect(() => {
        ref.current = fn;
        // eslint-disable-next-line react-hooks/exhaustive-deps
    });

    return useCallback((...args: Args) => ref.current(...args), []);
};

@rkfg
Copy link
Contributor Author

rkfg commented Dec 20, 2022

I think it should always fire:


The dependency array here includes the function that we pass and it will be different on each render (that's the problem we're solving after all!), plus all the dependencies we pass (but we pass an empty array so it doesn't matter). Am I missing something?

@fzaninotto
Copy link
Member

You're right, it will work, but the code isn't clear enough. I think passing a dependency that is always new to force a re-execution of the effect is a useless indirection. We should pass no dependency at all.

@rkfg
Copy link
Contributor Author

rkfg commented Dec 20, 2022

Okay, I'll add the shim you've proposed and use it instead, makes sense to signal the intent clearer + it might be useful for other things in the future.

@fzaninotto
Copy link
Member

wait, in fact you should replace the useEventCallback with the shim I suggested above. The current version actually executes at every render because the fn parameter is always new. So the dependency array is useless and should be removed.

Sorry for the back and forth, but this is the part of React I'm the less comfortable with.

@slax57
Copy link
Contributor

slax57 commented Dec 20, 2022

this is the part of React I'm the less comfortable with.

I believe we all are 😅

@rkfg
Copy link
Contributor Author

rkfg commented Dec 20, 2022

No problem, I'm always happy to learn. I'll see what I can do tomorrow.

@rkfg
Copy link
Contributor Author

rkfg commented Dec 21, 2022

Just to confirm, I'll also remove the dependencies here while replacing the hook:

const set = useEventCallback(

This lambda is recreated on each render so the dependency array is indeed useless. The only situation it'd be useful is if the function that's passed to useEventCallback is itself memoized (with useCallback for example) but this useStore hook is the only one that uses useEventCallback so I guess it's safe to make the replacement.

@fzaninotto
Copy link
Member

Seems good to me. I still have to test that it actually reduces re-renders using the React profiler.

@fzaninotto
Copy link
Member

Happy New Year! I finally took some time to test your PR.

I didn't find any different while using the simple example with or without your changes. Could you please set up a story that demonstrates the change?

Also, this is technically a new feature, so please rebase to PR against next.

@rkfg
Copy link
Contributor Author

rkfg commented Jan 11, 2023

Happy New Year! I rebased on next. I suppose it'd be hard to provide a working example, in my case I use a complex component that completely replaces the edit view of a resource, it's also put into its own context so re-renders might be triggered by many things including that context value change.

The code that caused issues was my custom hook that marks the currently opened item as "seen" after 5 seconds if the user doesn't navigate away. So that hook starts a timer in useEffect (the effect cleanup function cancels this timer) and when it fires, it updates some data using the callback that useUpdate returns. Since the timer works inside useEffect and uses that callback I have to pass it as one of the dependencies. If for whatever reason this component re-renders, useUpdate returns a different instance of the mutation callback which causes the effect to cleanup (and cancel the timer), then start another timer. I noticed that some re-renders happen when the user scrolls my custom component and that delays the timer. I realize that it's of course not a react-admin's problem, but the only dependency of that useEffect that changes is exactly the mutation callback which should retain referential equality between renders, and my patch fixes this.

I'm truly sorry but I can't come up with a natural example of these re-renders to occur during the more or less classic RA usage, the default list/edit/show components don't re-render spontaneously. And my custom component is too big and niche to adapt it for an example. Most of the time the current behavior shouldn't hit users I think but eventually it will.

@fzaninotto fzaninotto merged commit 92b2a44 into marmelab:next Jan 11, 2023
@fzaninotto
Copy link
Member

Thanks!

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

Successfully merging this pull request may close these issues.

Mutation hooks return non-memoized functions
4 participants