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

useEffect callback never called #17066

Closed
pschiffmann opened this issue Oct 11, 2019 · 16 comments
Closed

useEffect callback never called #17066

pschiffmann opened this issue Oct 11, 2019 · 16 comments

Comments

@pschiffmann
Copy link

I want to report a bug. My problem is that the callback function I pass to useEffect() is never called in my app. I uploaded a minimal app here that reproduces the issue.

As you can see, I render a couple of nested components. After the initial render, I update the component state inside a useEffect() callback. However, this callback is only executed for the first two components, not for the third level component. The result is that the third and subsequent levels are not rendered at all.

I suspect that this is a bug in React and not a misuse on my side because doing any of the following changes will let the component tree render correctly in all levels:

  • Don't use multiple React roots. If I remove the last (yellow) ReactDOM.render() call, then the second (red) component tree will render correctly.
  • Don't conditionally render child components. Removing the message !== DEFAULT_MESSAGE check (main.tsx, line 20) causes the component trees to render correctly.
  • Use useLayoutEffect() instead of useEffect().

If you need additional information to reproduce the issue or have any questions, let me know. I'd like provide any help I can!

@kunukn
Copy link
Contributor

kunukn commented Oct 11, 2019

I recreated this in https://codesandbox.io/s/react-typescript-4blmv

These is a lint warning that might help why this doesn't as intended work.

React Hook useEffect contains a call to 'setMessage'. Without a list of dependencies, this can lead to an infinite chain of updates. To fix this, pass [level] as a second argument to the useEffect Hook. (react-hooks/exhaustive-deps)

These lines should be changed as this leads to infinite look. In useEffect you are updating the state which triggers the useEffect, which updates the state, which triggers the useEffect .. repeatedly.

const [message, setMessage] = React.useState(DEFAULT_MESSAGE);
  React.useEffect(() => {
    const newMessage = `Level ${level}`;
    console.log(newMessage);
    setMessage(newMessage);
  });

@pschiffmann
Copy link
Author

pschiffmann commented Oct 11, 2019

@kunukn Sorry, I forgot to add an effect condition. I changed the useEffect() call (see diff), but the result stays the same: Components level 3 and beyond don't render.

edit: Tested my change in your codesandbox, saw that I missed the level variable as an effect dependency, updated my repository again. Still the same error - as you can see from the missing console.log() output, the effect callback doesn't even get called once.

@vkurchatkin
Copy link

vkurchatkin commented Oct 11, 2019

Here is somewhat minimal reproduction:

function useTest() {
  const [effect, setEffect] = useState(false);
  useEffect(() => {
    setEffect(true);
  }, []);

  return effect;
}

function A() {
  const effect = useTest();

  return (
    <div >
      {"" + effect}
      {effect && <B/>}
    </div>
  );
}

function B() {
  const effect = useTest();

  return (
    <div >
      {"" + effect}
      {effect && <C/>}
    </div>
  );
}


function C() {
  const effect = useTest();

  return (
    <div >
      {"" + effect}
    </div>
  );
}

function Other() {
  useEffect(() => {}, []);
  return null;
}


ReactDOM.render(
  <A/>,
  document.querySelector("#root1")
);

ReactDOM.render(
  <Other/>,
  document.querySelector("#root2")
);

Some observations:

  • If you change the order of render()s, it works
  • If you use useLayoutEffect, it works
  • If Other doesn't call useEffect, it works
  • If the second render() is delayed, it works
  • If B or C are rendered unconditionally, it works
  • If setEffect is delayed, it works

@WxSwen
Copy link

WxSwen commented Nov 4, 2019

i met this issue too.
if i use useLayoutEffect or fallback to class by use componentdidmount will be done.
if you solve this problem, tell me;

@pschiffmann
Copy link
Author

pschiffmann commented Nov 4, 2019

@WxSwen: I have no solution for the bug, but I found a workaround that works for me. Depending on your requirements, it might or might not help you, but I thought I might just share it. :)

It seems the problem occurs when I have multiple React instances on the page (multiple ReactDOM.render() calls). And shortly after I found this bug and opened the issue, I also ran into another challenge with multiple React instances: I wanted to share context between all components on the page, but for that to work, they must be in the same React render tree.

So what I ended up with was to go back to a single React instance. Instead, I call ReactDOM.render() only once, render into a disconnected <div>, and use React portals to render my individual components into their respective places on the page. For example:

const appRoot = document.createElement("div"); // This div is never attached to the DOM
const comp1Root = document.querySelector("#root1");
const comp2Root = document.querySelector("#root2");
ReactDOM.render(
  <>
    {/* I can place shared context providers here, if I want */}
    {ReactPortal.create(<Comp1 />, comp1Root, "root1")}
    {ReactPortal.create(<Comp2 />, comp2Root, "root2")}
  </>,
  appRoot
);

@WxSwen
Copy link

WxSwen commented Nov 4, 2019

@pschiffmann Even might not what i want but thanks for your kind and solution

@aleclarson
Copy link
Contributor

aleclarson commented Nov 10, 2019

I've hit this bug with React v16.9 and React Native, using two AppRegistry.registerComponent calls, which is the RN equivalent of two ReactDOM.render calls in the sense that they create a "React root".

Never mind. Failed to confirm this bug in React Native with the code provided by @vkurchatkin.

@gaearon
Copy link
Collaborator

gaearon commented Nov 10, 2019

This looks bad.

@gaearon
Copy link
Collaborator

gaearon commented Nov 10, 2019

Does someone want to turn this into a failing test case for ReactHooks-test.internal.js?

@aleclarson
Copy link
Contributor

aleclarson commented Nov 10, 2019

Does someone want to turn this into a failing test case for ReactHooks-test.internal.js?

#17335, but it's not failing as expected yet... 😅

Based on the code @vkurchatkin posted above.

edit: I've just tested on react-native-macos, and the demo in #17335 works fine, so I guess this bug doesn't affect me, which means I'm done helping on this.

@aleclarson
Copy link
Contributor

aleclarson commented Nov 11, 2019

Downgrading to React DOM 16.8 is a valid workaround.

acdlite added a commit to acdlite/react that referenced this issue Nov 11, 2019
aleclarson added a commit to alloc/wana that referenced this issue Nov 12, 2019
acdlite added a commit to acdlite/react that referenced this issue Nov 12, 2019
The bug
-------

In a multi-root app, certain passive effects (`useEffect`) are never
fired. See facebook#17066.

The underlying problem
----------------------

The implicit contract of `flushPassiveEffects` is that, right after
calling it, there should be no pending passive effects. In the normal
case, in concurrent mode, this is true. But the current implementation
fails to account for the case where a passive effect schedules
synchronous work, which in turn schedules additional passive effects.

This led to `rootWithPendingPassiveEffects` being overwritten in the
commit phase, because an assignment that assumed it was replacing null
was actually replacing a reference to another root, which has the
consequence of dropping passive effects on that root.

The fix
-------

The fix I've chosen here is, at the beginning of the commit phase, keep
flushing passive effects in a loop until there are no more.

This doesn't not change the "public" implementation of
`flushPassiveEffects`, though it arguably should work this way, too. I
say "public" because it's only used by implementation layers on top of
React which we control: mainly, the legacy version of `act` that does
not use the mock Scheduler build. So there's probably still a bug
in that `act` implementation.

I will address `act` in a follow-up. The ideal solution is to replace
the legacy `act` with one implemented directly in the renderer, using a
special testing-only build of React DOM. Since that requires a breaking
change, we'll need an interim solution. We could make the "public" `act`
recursively flush effects in a loop, as I've done for the commit phase.
However, I think a better solution is to stop automatically flushing the
synchronous update queue at the end of `flushPassiveEffects`, and
instead require the caller to explicitly call `flushSyncUpdateQueue` (or
the equivalent) if needed. This follows the same pattern we use
internally in the work loop, which is designed to avoid factoring
hazards like the one that resulted in this bug.
acdlite added a commit that referenced this issue Nov 12, 2019
…root app (#17347)

* Regression test: Effects dropped across roots

See #17066

* [Bugfix] Passive effects loop

The bug
-------

In a multi-root app, certain passive effects (`useEffect`) are never
fired. See #17066.

The underlying problem
----------------------

The implicit contract of `flushPassiveEffects` is that, right after
calling it, there should be no pending passive effects. In the normal
case, in concurrent mode, this is true. But the current implementation
fails to account for the case where a passive effect schedules
synchronous work, which in turn schedules additional passive effects.

This led to `rootWithPendingPassiveEffects` being overwritten in the
commit phase, because an assignment that assumed it was replacing null
was actually replacing a reference to another root, which has the
consequence of dropping passive effects on that root.

The fix
-------

The fix I've chosen here is, at the beginning of the commit phase, keep
flushing passive effects in a loop until there are no more.

This doesn't not change the "public" implementation of
`flushPassiveEffects`, though it arguably should work this way, too. I
say "public" because it's only used by implementation layers on top of
React which we control: mainly, the legacy version of `act` that does
not use the mock Scheduler build. So there's probably still a bug
in that `act` implementation.

I will address `act` in a follow-up. The ideal solution is to replace
the legacy `act` with one implemented directly in the renderer, using a
special testing-only build of React DOM. Since that requires a breaking
change, we'll need an interim solution. We could make the "public" `act`
recursively flush effects in a loop, as I've done for the commit phase.
However, I think a better solution is to stop automatically flushing the
synchronous update queue at the end of `flushPassiveEffects`, and
instead require the caller to explicitly call `flushSyncUpdateQueue` (or
the equivalent) if needed. This follows the same pattern we use
internally in the work loop, which is designed to avoid factoring
hazards like the one that resulted in this bug.
@acdlite
Copy link
Collaborator

acdlite commented Nov 14, 2019

Fix has landed in the Next channel: https://codesandbox.io/s/react-typescript-2thgm

I'll cut a release later today. Thanks for reporting!

@acdlite acdlite closed this as completed Nov 14, 2019
@acdlite
Copy link
Collaborator

acdlite commented Nov 15, 2019

Fix released in 16.12.0

@wahyuutomoputra
Copy link

https://github.com/react-navigation/hooks#only-for-react-navigation-v3--v4-not-v5

@poddarkhushbu07
Copy link

poddarkhushbu07 commented Oct 11, 2021

Facing this same issue in version 17.0.2

I am calling useEffect in App.js with [] dependencies

Never mind

Solved this by replacing

ReactDOM.render(
  <React.StrictMode>
    
  </React.StrictMode>,
    document.getElementById("root")
);

To

const rootElement = document.getElementById("root");
ReactDOM.render(
  <React.StrictMode>
    
  </React.StrictMode>,
    rootElement
);

@yairEO
Copy link

yairEO commented Jan 29, 2022

@poddarkhushbu07 - your "solution" makes no sense and both code pieces are identical in practice

Alternatively, you can just write:

ReactDOM.render(<App/>, root); // where "root" is an element id

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

No branches or pull requests

10 participants