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

Bug: useEffect runs twice on component mount (StrictMode, NODE_ENV=development) #24502

Closed
cytrowski opened this issue May 5, 2022 · 41 comments
Closed
Labels
Resolution: Expected Behavior Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@cytrowski
Copy link

React version: 18.0.x, 18.1.x, 18.2.x

Steps To Reproduce

  1. Visit provided sandbox
  2. Open console and observe logs displayed twice.
  3. Click the button and observe the rendering log happens twice, the effect log happens once.

Link to code example: https://codesandbox.io/s/react-18-use-effect-bug-iqn1fx

The current behavior

The useEffect callback runs twice for initial render, probably because the component renders twice. After state change the component renders twice but the effect runs once.

The expected behavior

I should not see different number of renders in dev and prod modes.

Extras

The code to reproduce:

import { useEffect, useReducer } from "react";
import "./styles.css";

export default function App() {
  const [enabled, toggle] = useReducer((x) => !x, false);

  useEffect(() => {
    console.log(
      "You will see this log twice for dev mode, once after state change - double effect call"
    );
  }, [enabled]);

  console.log("You will see this log twice for dev mode - double rendering");

  return (
    <div className="App">
      <h1>Hello CodeSandbox</h1>
      <h2>Start editing to see some magic happen!</h2>
      <button onClick={() => toggle()}>
        Toggle me: {enabled ? "on" : "off"}
      </button>
    </div>
  );
}
@cytrowski cytrowski added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label May 5, 2022
@eps1lon
Copy link
Collaborator

eps1lon commented May 5, 2022

Effects firing twice in <React.StrictMode /> was added in React 18.

For more information, check out StrictMode: Ensuring reusable state

@cytrowski
Copy link
Author

Thank you @eps1lon for clarification. However this is not intuitive and I'd expect this being mentioned/referenced in the useEffect docs here: https://reactjs.org/docs/hooks-effect.html

I can imagine people being surprised by this behaviour. The perfect solution would be to have a warning in the console.

@cytrowski
Copy link
Author

Also: I see no mention of this breaking change in the changelog: https://github.com/facebook/react/blob/main/CHANGELOG.md#1800-march-29-2022

@sean-clayton
Copy link

@cytrowski https://github.com/facebook/react/blob/main/CHANGELOG.md#react-1 <- In the "stricter strict mode" bullet

@cytrowski
Copy link
Author

@cytrowski https://github.com/facebook/react/blob/main/CHANGELOG.md#react-1 <- In the "stricter strict mode" bullet

@sean-clayton this does not explain why I see the rendering log twice after state change (not only initial render). However I believe there is already an issue for that.

Anyway I still believe this should be something announced in the console warnings (similar way you do with encouraging React Dev Tools installation)

@gaearon
Copy link
Collaborator

gaearon commented May 5, 2022

It is described in the upgrade post. We strongly recommend anyone upgrading to read the full upgrade post.

https://reactjs.org/blog/2022/03/08/react-18-upgrade-guide.html#updates-to-strict-mode

I agree it's confusing this isn't documented explicitly in the conceptual introduction to effects. We'll fix this in the new docs. We've published some docs on this now:

I disagree about the console log. The DevTools log is different because it disappears by installing DevTools. So most users (who we presume install DevTools) won't get it.

@cytrowski
Copy link
Author

I agree it's confusing this isn't documented explicitly in the conceptual introduction to effects. We'll fix this in the new docs.

Thank you for that @gaearon :)

Any hints where to put fetch calls so that they are not triggered twice in dev mode useEffect callback?

@gaearon
Copy link
Collaborator

gaearon commented May 5, 2022

The "best" advice depends on how your app is built:

  • If you use a framework (e.g. Next.js / Remix / etc), use that framework's built-in data fetching mechanisms. They're usually much better than ad-hoc effects because they handle caching, server rendering, and avoid waterfalls.
  • If you use a client-side cache (e.g. React Query / useSWR / etc), use that cache's data fetching mechanisms.
  • If you don't want to consider either a framework or a client-side cache, please keep fetching in effects. But there are numerous reasons why you might want to consider the solutions above! (For example, if you don't have any caching solution, clicking Back button in your app would always show a spinner instead of cached data—an easy way to detect apps that call fetch + setState directly in their effects.)

This advice is not new and is not specific to React 18, but maybe we haven't shared it very clearly before. I've written a longer and more nuanced version in this Reddit reply. I hope this clarifies the intention behind sharing this advice a bit better.

If you fetch from effects, you can do something like this:

useEffect(() => {
  let ignore = false;
  fetchStuff().then(res => {
    if (!ignore) setResult(res)
  })
  return () => { ignore = true }
}, [])

This will not prevent the double-fetch, but it will ignore the result of the first one. So it's like it never happened. There is no harm from the extra fetch call in development. You could also use a fetching helper with cancellation, and cancel the fetch instead of ignoring its result. Keep in mind that in production you'll only get one fetch.

Read more:

@cytrowski
Copy link
Author

cytrowski commented May 5, 2022

For posterity:

Just out of curiosity I went and check how react-query solves this problem and it's impressive how many things they handle out of the box.

To prove the problem doesn't occur I created this sandbox: https://codesandbox.io/s/react-query-vs-react-18-strict-mode-xpzkr1

Once again thanks for explaining the problem @gaearon

Case closed ;)

@CharlyJazz
Copy link

So basically, in React 18 is a better idea install a third party library to do API fetching.

@TkDodo
Copy link

TkDodo commented May 9, 2022

Hi, react-query maintainer here. 👋

glad to see people are happy with react-query. 🙌
just for completeness, I'd like to point out that even in react-query, the strict effects fires the fetch "twice". We just deduplicate multiple requests that are going on at the same time, so it literally doesn't matter if one component mounts, then unmounts, then mounts again, or if 5 consumers call useQuery at the same time.

strict-effects have also shown us two edge-case bugs already, so I'm really glad that exists. 🙏

You could also use a fetching helper with cancellation, and cancel the fetch instead of ignoring its result.

react-query supports AbortSignals, so if that is used, what will happen is that the first fetch is aborted and the second fetch will supersede the first one. This might also throw people off because there are two requests in the network tab, but it's how aborting queries is designed to work :)

@cytrowski
Copy link
Author

@TkDodo I've seen that it's not just some clever hack around useEffect - instead it's nicely deduped :) Thanks for making that clear ;)

@gaearon
Copy link
Collaborator

gaearon commented May 9, 2022

The mental model for the behavior is "what should happen if the user goes to page A, quickly goes to page B, and then switches to page A again". That's what Strict Mode dev-only remounting verifies.

If your code has a race condition when called like this, it will also have a race condition when the user actually switches between A, B, and A quickly. If you don't abort requests or ignore their results, you'll also get duplicate/conflicting results in the A -> B -> A scenario.

@cytrowski
Copy link
Author

@gaearon That's really clever, however I'd expect something like that being an opt-in feature of strict mode anyway. It's a surprising behaviour for the components that by design are not remounted during my app lifecycle (eg. some root-level context providers). It's just a personal opinion though. I can imagine turning it on/off by some extra prop. However I see that for the simplicity it's much simpler to just let it be there.

@TkDodo
Copy link

TkDodo commented May 9, 2022

I can imagine turning it on/off by some extra prop

This is exactly what <StrictMode> is doing though, isn't it? If you don't want it - don't wrap your app in StrictMode

@yangmingshan
Copy link

yangmingshan commented May 9, 2022

The "best" advice is not to fetch from useEffect at all. There are many reasons not to (fetch on render leads to waterfalls, you start fetching too late which is inefficient, you don't have a good place to cache the result between components, there is no deduplication between requests, etc). So I'd recommend a library like React Query or similar instead.

If you fetch from effects, you can do something like this:

useEffect(() => {
  let ignore = false;
  fetchStuff().then(res => {
    if (!ignore) setResult(res)
  })
  return () => { ignore = true }
}, [])

This will not prevent the double-fetch, but it will ignore the result of the first one. So it's like it never happened. There is no harm from the extra fetch call in development. You could also use a fetching helper with cancellation, and cancel the fetch instead of ignoring its result. Keep in mind that in production you'll only get one fetch.

But is this isMountedRef? I thought you said isMountedRef are evil in reactwg/react-18#82

Edit:
Oh! I think I get it. Using isMountedRef in effect is fine, but using it in event handler is risky. Is it?

@gaearon
Copy link
Collaborator

gaearon commented May 9, 2022

@yangmingshan This is not a ref, just a local variable specific to that particular effect. Rather than to the component lifetime like a ref.

@yangmingshan
Copy link

@yangmingshan This is not a ref, just a local variable specific to that particular effect. Rather than to the component lifetime like a ref.

But if I change it to ref:

const isUnmountedRef = useRef(false)
useEffect(() => {
  isUnmountedRef.current = false
  fetchStuff().then(res => {
    if (!isUnmountedRef.current) setResult(res)
  })
  return () => { isUnmountedRef.current = true }
}, [])

Are they same?

@gaearon
Copy link
Collaborator

gaearon commented May 9, 2022

No, they're not the same. In my example, ignore is local to that specific call of the effect. It doesn't affect other calls. In your example, the ref affects all effect calls.

@cytrowski
Copy link
Author

I can imagine turning it on/off by some extra prop

This is exactly what <StrictMode> is doing though, isn't it? If you don't want it - don't wrap your app in StrictMode

I assume that the StrictMode does more than that. And it wasn't doing that in React 17. I don't argue - I'm just curious why this specific breaking behaviour couldn't have been added as opt-in. I assume there are good reasons behind that.

Anyway - I don't need more explanations - will just stick to React 17 for a while longer.

To give you more context: I used to teach React classes + I know a bunch of people who do it too. I can imagine their confusion when they try to explain useEffect during some live coding session with CRA-bootstrapped project and the code is not behaving as they expect. I wouldn't stress about it if it was explicitly stated in the docs of useEffect.

Also I find it very common in recruitment coding assignments from the category: "show me you can fetch some data from the REST API". I can imagine both the recruiter and the candidate being surprised why the fetch is being called twice (assuming they work with React 17 projects right now and used CRA just for the sake of the live coding task).

@gaearon
Copy link
Collaborator

gaearon commented May 9, 2022

I wouldn't stress about it if it was explicitly stated in the docs of useEffect.

Would you like to send a PR? I totally agree the docs should mention this.

@cytrowski
Copy link
Author

I think I will find some time this week @gaearon :) Thanks - will do the PR. Should I tag it with this issue number?

@gaearon
Copy link
Collaborator

gaearon commented May 9, 2022

Yep!

@cytrowski
Copy link
Author

It took me less than one week 😉 Hope those 2 paragraphs are enough to make everyone happy 😄 reactjs/react.dev#4650

@FokinAleksandr
Copy link

There are many reasons not to (fetch on render leads to waterfalls, you start fetching too late which is inefficient, you don't have a good place to cache the result between components, there is no deduplication between requests, etc). So I'd recommend a library like React Query or similar instead.

But react-query also uses fetch-on-render approach which is not good as you mentioned

@gaearon
Copy link
Collaborator

gaearon commented May 10, 2022

Yeah, that’s not ideal! But it does offer a method for prefetching and priming the cache. It also has support for server rendering.

@TkDodo
Copy link

TkDodo commented May 10, 2022

But react-query also uses fetch-on-render approach which is not good as you mentioned

I tried to ask around regarding the difference between fetch-on-render and render-as-you-fetch. The details still seem pretty fuzzy to me, but react-query is well equipped to handle both, with or without suspense.

render-as-you-fetch seems to be just a fancy phrase for saying: trigger the fetch manually before you render the component, then react can render the rest in the meantime. This is what queryClient.prefetchQuery offers. At that point, both Suspense and ErrorBoundaries basically just offer a way to have more global loading spinners / error handlings - and both concepts are supported in react-query, independently of each other :)

As far as I can see, even with render-as-you-fetch, if you don't prefetch and read from two different resources in the same component tree, you will wind up with a waterfall.

@moinulmoin
Copy link

To prove the problem doesn't occur I created this sandbox: https://codesandbox.io/s/react-query-vs-react-18-strict-mode-xpzkr1

I checked your sandbox link. It's still rendering twice. @cytrowski Can you please describe how React Query solves this issue? is it fetching data once?

image

@iarmankhan
Copy link

iarmankhan commented May 19, 2022

The "best" advice is not to fetch from useEffect at all. There are many reasons not to (fetch on render leads to waterfalls, you start fetching too late which is inefficient, you don't have a good place to cache the result between components, there is no deduplication between requests, etc). So I'd recommend a library like React Query or similar instead.

If you fetch from effects, you can do something like this:

useEffect(() => {
  let ignore = false;
  fetchStuff().then(res => {
    if (!ignore) setResult(res)
  })
  return () => { ignore = true }
}, [])

This will not prevent the double-fetch, but it will ignore the result of the first one. So it's like it never happened. There is no harm from the extra fetch call in development. You could also use a fetching helper with cancellation, and cancel the fetch instead of ignoring its result. Keep in mind that in production you'll only get one fetch.

@gaearon What would you suggest to do otherwise? I know about libraries like react-query which does not fetch in useEffect, but is there any other solution (without using any third-party library)?

Thanks

@TkDodo
Copy link

TkDodo commented May 19, 2022

I checked your sandbox link. It's still rendering twice. @cytrowski Can you please describe how React Query solves this issue? is it fetching data once?

rendering twice is not the same as fetching twice. if you look at the network tab, there is only one request going out. strict mode still renders the component twice, and since v18, it does not silence the console on the second run, so you will see double logs.

@gajus
Copy link
Contributor

gajus commented May 20, 2022

Just use something like this:

import { type EffectCallback, useEffect, useRef } from 'react';

export const useMount = (effect: EffectCallback) => {
  const mounted = useRef(false);

  useEffect(() => {
    if (mounted.current) {
      return effect();
    }

    mounted.current = true;

    return () => {};
  }, [mounted, effect]);
};

We mostly use this pattern for doing redirects and pages with side-effects, like log-out.

@kuzux
Copy link

kuzux commented May 22, 2022

For API fetching, if we do not wish to include another third party dependency, where exactly should we do the fetching if not in use effect?

The official docs at https://reactjs.org/docs/faq-ajax.html say componentDidMount (Which, I guess shows that it hasn't been updated in a while). Wasn't componentDidMount equivalent to a use effect with empty dependencies?

@Ashuto7h-0
Copy link

Ashuto7h-0 commented May 28, 2022

useEffect(() => {
  let ignore = false;
  fetchStuff().then(res => {
    if (!ignore) setResult(res)
  })
  return () => { ignore = true }
}, [])

Is it only me or anyone also has this doubt - why the ignore variable will not be reinitialised to false when component is unmounted and mounted again on initial render in development?

@gaearon could you put some light on this...

@PatrissolJuns
Copy link

Just use something like this:

import { type EffectCallback, useEffect, useRef } from 'react';

export const useMount = (effect: EffectCallback) => {
  const mounted = useRef(false);

  useEffect(() => {
    if (mounted.current) {
      return effect();
    }

    mounted.current = true;

    return () => {};
  }, [mounted, effect]);
};

We mostly use this pattern for doing redirects and pages with side-effects, like log-out.

Please how do you use it?

@cytrowski
Copy link
Author

cytrowski commented May 28, 2022

Just use something like this:

import { type EffectCallback, useEffect, useRef } from 'react';

export const useMount = (effect: EffectCallback) => {
  const mounted = useRef(false);

  useEffect(() => {
    if (mounted.current) {
      return effect();
    }

    mounted.current = true;

    return () => {};
  }, [mounted, effect]);
};

We mostly use this pattern for doing redirects and pages with side-effects, like log-out.

Please how do you use it?

I bet you can just

const MyFancyComponent = () => {
  useMount(() => console.log('The component is mounted'));
  return <div>Hello</div>
}

or if you want to do something on "unmount":

const MyFancyComponent = () => {
  useMount(() => {
    console.log('The component is mounted');
    return () => console.log('The component unmounts');
  });
  return <div>Hello</div>
}

@motevallian
Copy link

useEffect(() => {
  let ignore = false;
  fetchStuff().then(res => {
    if (!ignore) setResult(res)
  })
  return () => { ignore = true }
}, [])

Is it only me or anyone also has this doubt - why the ignore variable will not be reinitialised to false when component is unmounted and mounted again on initial render in development?

@gaearon could you put some light on this...

The ignore variable is just there to avoid setResult(rest) if the component is unmounted before the promise is fulfilled. As @gaearon said this variable will NOT prevent fetch from being called twice, but prevents the first one setting the result while React is unmounting and remounting for the second time.
The second fetch though perhaps manages to call setResult as no unmount would happen afterwards.

@Ashuto7h-0
Copy link

@motevallian thanks for this awesome explaination

@morzel85
Copy link

Currently the thread focuses almost completely on fetching, but there is another side of the story: behavior of complex components: #24553

What worries me is the fact that development mode most people will run (StrictMode is there in CRA) is getting further from production mode. So area for making subtle errors that disappear as soon as you start troubleshooting is increasing.

@shivamjjha
Copy link
Contributor

shivamjjha commented Jun 5, 2022

To prove the problem doesn't occur I created this sandbox: https://codesandbox.io/s/react-query-vs-react-18-strict-mode-xpzkr1

I checked your sandbox link. It's still rendering twice. @cytrowski Can you please describe how React Query solves this issue? is it fetching data once?

image

Yeah, if you check network tab, you can see that the request goes only once as mentioned here.
The console logs shows the subsequent re-renders that React 18 brings in.

So suppose, even if a user goes like A -> B -> A, react-query still handles it; it does not fetch twice.

If you want to fetch before you render, you can use prefetch client, as explained here; or fetch server side. React Query supports both
@moinulmoin

@cytrowski
Copy link
Author

To prove the problem doesn't occur I created this sandbox: https://codesandbox.io/s/react-query-vs-react-18-strict-mode-xpzkr1

I checked your sandbox link. It's still rendering twice. @cytrowski Can you please describe how React Query solves this issue? is it fetching data once?
image

Yeah, if you check network tab, you can see that the request goes only once as mentioned here. The console logs shows the subsequent re-renders that React 18 brings in.

So suppose, even if a user goes like A -> B -> A, react-query still handles it; it does not fetch twice.

If you want to fetch before you render, you can use prefetch client, as explained here; or fetch server side. React Query supports both @moinulmoin

The second log is there because React 17+ batches all the logs in dev mode and prints them again at the end. Open this sandbox in separate tab and see the difference in browser console. Codesandbox does not distinct between those two - the browser console does.

@gaearon
Copy link
Collaborator

gaearon commented Jun 16, 2022

We've documented this in detail on the Beta website.

I've also responded on Reddit to a question about how to fetch data in React 18 with some historical context.

Hope this is helpful. Sorry some links from there are 404 because other pages are still being written.

I'll lock this thread since we have a canonical answer and I don't want it to get lost in the follow-up comments. But if there's some pattern the new doc doesn't cover please let us know in a new issue.

@facebook facebook locked as resolved and limited conversation to collaborators Jun 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Resolution: Expected Behavior Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests