facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
227.97k stars 46.53k forks source link

In Strict Mode, function component and class component are inconsistent with scenes that produce side effects #23015

Open y805939188 opened 2 years ago

y805939188 commented 2 years ago

React version:

17.0.2

Steps To Reproduce

import React from 'react';

const TestFunc = () => {
  debugger
  const myRef = React.useRef(true)
  if (myRef.current) {
    myRef.current = false
  }

  return <div>666</div>
}

class TestClass extends React.Component {
  myRef = React.createRef()
  constructor(props) {
    super(props);
    this.myRef.current = true
  }

  render() {
    debugger
    if (this.myRef.current) {
      this.myRef.current = false
    }
    return <div>888</div>
  }
}

function App() {
  return (
    <React.StrictMode>
      <TestFunc />
      <TestClass />
    </React.StrictMode>
  );
}

export default App;
  1. Use strict mode
  2. Observe the changes of the myRef.current variable when the two components are rendered for the second time
  3. In the function component, the value of the myRef.current variable is "true" in the two renders before and after, so the two renders will go to the "myRef.current = false" logic. But in the class component, when rendering twice, the myRef.current variable is "true" in the first rendering, but the "false" that was modified in the first rendering is retained in the second rendering.

Reason: I looked at the source code and found that for the function component, although renderWithHooks was called twice before and after and the same workInProgress was used, but every time in the "renderWithHooks" method, the value of "workInProgress.memoizedState" is set to null, and in the useRef function of the mounting phase, the "workInProgress.memoizedState" property on "alternate" will not be reused, therefore, in the mounting phase in strict mode, during the second rendering, the state saved by ref during the first rendering cannot be obtained in the function component. 4641640144553_ pic

But in the scenario of the class component, the same "instance" is used when the render method is called twice before and after, so the "this" in the render method two times before and after all points to the same one "instance". 4651640144635_ pic

Expected: I think that since the role of strict mode is to make multiple calls so that developers can find that using side effects in some life cycles may cause some problems, should the phenomenon of the class component be consistent with the phenomenon of the function component?

Thanks~:pray::pray::pray:

bvaughn commented 2 years ago

This is an interesting observation so I've tagged it for discussion. (Not sure it's something we'll change though.)

Your investigation above is correct, but missing one detail– in strict mode, React instantiates class components twice (to tease out side effects in the constructor): https://github.com/facebook/react/blob/bcd24a6706cd802e49e9c0a509a42893ddceca7b/packages/react-reconciler/src/ReactFiberClassComponent.new.js#L656-L670

The primary difference is that it throws one of the two instances away and calls render twice on the single remaining instance– so the class instance ref isn't reset between those two renders.

This is kind of an artifact of the way the code is structured. (It would have been a much more pervasive change to do the larger instantiate-and-render loop twice.)

y805939188 commented 2 years ago

This is an interesting observation so I've tagged it for discussion. (Not sure it's something we'll change though.)

Your investigation above is correct, but missing one detail– in strict mode, React instantiates class components twice (to tease out side effects in the constructor):

https://github.com/facebook/react/blob/bcd24a6706cd802e49e9c0a509a42893ddceca7b/packages/react-reconciler/src/ReactFiberClassComponent.new.js#L656-L670

The primary difference is that it throws one of the two instances away and calls render twice on the single remaining instance– so the class instance ref isn't reset between those two renders.

This is kind of an artifact of the way the code is structured. (It would have been a much more pervasive change to do the larger instantiate-and-render loop twice.)

Thanks for reminding me of this detail. I took a look again, and indeed the "instance" that was instantiated for the first time was directly discarded in strict mode. Regarding this situation, is it possible to find a way to get the instance generated by the first initialization when the render method is called, and then use different instances for the two renders?(I think because the code structure, it may be difficult to pass both instances down.:pensive::pensive::pensive:)

In my scenario, I found that someone make side-effect operations in class "render" function. I originally thought it would be dangerous, but found that he did bypass the danger through the variable on this (But I still feel that this is not a good design).

I think that it is better to unify the behavior of the function component and the class component.I want to know if the senior in the react team plan to modify this situation.

Thanks~

cuobiezi commented 2 years ago

Refer from https://reactjs.org/docs/implementation-notes.html

If App is a function, the reconciler will call App(props) to get the rendered element.

If App is a class, the reconciler will instantiate an App with new App(props), call the componentWillMount() lifecycle method, and then will call the render() method to get the rendered element.

I think function components should be designed as some simple component just included render part. Maybe we can imagine function component is class component's render function. Actually your demo can be traslate to fellow code:



const TestFunc = () => {
  debugger
  const myRef = React.useRef(true)
  if (myRef.current) {
    myRef.current = false
  }

  return <div>666</div>
}
// Traslate function component to class component , Actaully it is diff with class component TestClass
class _TestFunc extends React.Component {
     render () {
        const myRef = React.useRef(true)
        if (myRef.current) {
          myRef.current = false
        }
        return <div>666</div>
     }
}

class TestClass extends React.Component {
  myRef = React.createRef()
  constructor(props) {
    super(props);
    this.myRef.current = true
  }

  render() {
    debugger
    if (this.myRef.current) {
      this.myRef.current = false
    }
    return <div>888</div>
  }
}

function App() {
  return (
    <React.StrictMode>
      <TestFunc />
      <TestClass />
    </React.StrictMode>
  );
}

export default App;
y805939188 commented 2 years ago

Refer from https://reactjs.org/docs/implementation-notes.html

If App is a function, the reconciler will call App(props) to get the rendered element.

If App is a class, the reconciler will instantiate an App with new App(props), call the componentWillMount() lifecycle method, and then will call the render() method to get the rendered element.

I think function components should be designed as some simple component just included render part. Maybe we can imagine function component is class component's render function. Actually your demo can be traslate to fellow code:

const TestFunc = () => {
  debugger
  const myRef = React.useRef(true)
  if (myRef.current) {
    myRef.current = false
  }

  return <div>666</div>
}
// Traslate function component to class component , Actaully it is diff with class component TestClass
class _TestFunc extends React.Component {
     render () {
        const myRef = React.useRef(true)
        if (myRef.current) {
          myRef.current = false
        }
        return <div>666</div>
     }
}

class TestClass extends React.Component {
  myRef = React.createRef()
  constructor(props) {
    super(props);
    this.myRef.current = true
  }

  render() {
    debugger
    if (this.myRef.current) {
      this.myRef.current = false
    }
    return <div>888</div>
  }
}

function App() {
  return (
    <React.StrictMode>
      <TestFunc />
      <TestClass />
    </React.StrictMode>
  );
}

export default App;

Yes~ Due to the current implementation of class components in strict mode, it is true that translating function components into class components can record side effects. But I am thinking, is it in suspense mode or concurrent mode, even if it is not in strict mode, if side effects are placed in render, some problems will arise.

In this example: https://codesandbox.io/s/suspicious-estrela-rjo1x?file=/src/App.js:0-791

This example is not in strict mode. Among them, when the sentence "resource.read(true, 1000, true)" is executed, the "react-suspense-cache" component will throw a promise to interrupt the rendering. When react is rendered for the second time, the state saved by the ref in the previous rendering is also lost.

In fact, this example is really extreme, I just wrote this example purely to create a problem. But it is indeed a problem that may occur in suspense mode, and at the same time react can also work normally.

I wrote this example just to show that it is dangerous to do side-effect operations in the render method. I think if react can be in strict mode, whether it is a function component or a class component, it can be completely isolated from the previous state, then it may be more Great to help developers find some problems.

Thanks~

import React, { Suspense } from "react";
import fetchData from "react-suspense-cache";

const getMockData = (time, fakeRes) =>
  new Promise((res) => {
    setTimeout(() => {
      res(fakeRes);
    }, time);
  });

const resource = fetchData(getMockData);

class Test extends React.Component {
  myRef = true;
  render() {
    if (this.myRef) {
      console.log(this.myRef);
      this.myRef = false;
    }

    const res = resource.read(true, 1000, true);

    return res ? <div>get remote data succeed</div> : <div>error</div>;
  }
}

function App() {
  return (
    <Suspense fallback={<div>loading...</div>}>
      <Test />
    </Suspense>
  );
}

export default App;