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

Hooks: callback in useEffect is not always fired in same time. #14354

Closed
morlay opened this issue Nov 29, 2018 · 3 comments
Closed

Hooks: callback in useEffect is not always fired in same time. #14354

morlay opened this issue Nov 29, 2018 · 3 comments

Comments

@morlay
Copy link

morlay commented Nov 29, 2018

Do you want to request a feature or report a bug?
maybe a bug?

What is the current behavior?
see bellow.

If the current behavior is a bug, please provide the steps to reproduce and if possible a minimal demo of the problem. Your bug will get fixed much faster if we can run your code and it doesn't have dependencies other than React. Paste the link to your JSFiddle (https://jsfiddle.net/Luktwrdm/) or CodeSandbox (https://codesandbox.io/s/new) example below:

import React from "react";
import { render } from "react-dom";
import { useState, useLayoutEffect, useEffect } from "react";

function App() {
  const [mounted, setMount] = useState("");

  useEffect(() => {
    console.log("App effect");
    setMount(s => s + "App mounted;");
  }, []);

  useLayoutEffect(() => {
    console.log("App layout effect");
    setMount(s => s + "App layout mounted;");
  }, []);

  return <div>{mounted}</div>;
}

function App2() {
  const [mounted, setMount] = useState("");

  useEffect(() => {
    console.log("App2 effect");
    setMount(s => s + "App2 mounted;");
  }, []);

  useLayoutEffect(() => {
    console.log("App2 layout effect");
  }, []);

  return <div>{mounted}</div>;
}

test("test App", () => {
  const node = mount(<App />);
  console.log("mounted", node.innerHTML);
});

test("test App2", () => {
  const node = mount(<App2 />);
  console.log("mounted", node.innerHTML);
});

function mount(e) {
  const node = document.createElement("div");
  render(e, node);
  return node;
}

Edit 3vv85q81x6
run the tests, and see the log.

What is the expected behavior?

App layout effect 
App effect 
mounted <div>App layout mounted;App mounted;</div> 
App2 layout effect 
App2 effect
mounted <div>App2 mounted;</div> 

But got

image

after useEffect callback firing in App2, we can't get re-rendered dom.
btw, in codesandbox, log the App2 effect, but in local node env, will lost the log App2 effect

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

  • react 16.7.0-alpha.2
  • react-dom 16.7.0-alpha.2
  • node 10.13.0
  • macOS mojave 10.14.1
@morlay morlay changed the title Hooks: question the time of firing useEffect Hooks: callback in useEffect is not always fired in same time. Nov 30, 2018
@KpjComp
Copy link

KpjComp commented Nov 30, 2018

I'm new to React, but your output to me looks correct.

In both cases we have ->
App layout effect
App effect
App2 layout effect
App2 effect

You can ignore the mounted parts, because it's not even part of the React component.

From what I can tell, React is not totally synchronous, it uses a fiber a stack. I'm assuming this is prevent successive DOM comparisons on multiple setStates etc.

So what I would suggest is that you treat mount();, like any Javascript asynchronous method. If you do this your output makes total sense.

In fact I wouldn't be surprised if you had a result like ->
App layout effect
App2 layout effect
App effect
App2 effect

Seen as your doing this on two separate mount points.

@morlay
Copy link
Author

morlay commented Dec 1, 2018

@KpjComp
it's different test cases, why i mount into different element.
I understand the ReactDOM render rules as you said.

This issue show useEffect may be triggered synchronous or asynchronous in different cases.
It is may make user confused to write testing cases.

I just hope it could be always asynchronous or always synchronous.

It is fine to forcing to write testing cases asynchronous.

test("test App", async () => {
  const node = await mount(<App />);
  console.log("mounted", node.innerHTML);
});

test("test App2", async () => {
  const node = await mount(<App2 />);
  console.log("mounted", node.innerHTML);
});

function mount(e) {
  return new Promise(resolve => {
    const node = document.createElement("div");
    render(e, node, () => {
     // todo have to wrapper setTimeout to make sure asynchronous
      setTimeout(() => {
        resolve(node);
      });
    });
  });
}

Edit 2z8k2o0rlp

But there have similar issue. The render callback of ReactDOM.render not always triggered asynchronous. (see the todo in code above)

@gaearon
Copy link
Collaborator

gaearon commented Jan 18, 2019

I think the request here is the same as #14050.

@gaearon gaearon closed this as completed Jan 18, 2019
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

3 participants