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: old JSX from previous rendering visible in strict mode #26628

Closed
MartinGeisse opened this issue Apr 14, 2023 · 6 comments
Closed

Bug: old JSX from previous rendering visible in strict mode #26628

MartinGeisse opened this issue Apr 14, 2023 · 6 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@MartinGeisse
Copy link

MartinGeisse commented Apr 14, 2023

React version: 18.2.0

I'm having a problem where React seems to show old JSX, i.e. not update the DOM with the latest JSX returned by a component.

The problem can be reproduced in React 18.2.0 with the following sample component. Please note that while this sample code does a few things that are not recommended, such as using global non-state flags and calling setTimeout() directly in the rendering function, to my understanding this should not cause the behaviour I'm getting.

import React, {useState} from "react";

let firstFlag = false;
let secondFlag = false;

export function ReproduceBug() {
  console.log("rendering starts...", firstFlag, secondFlag);
  const [, setDummyState] = useState({ a: "1" });

  console.log("reached first if-statement");
  if (!firstFlag) {
    firstFlag = true;
    setTimeout(() => {
      console.log("timeout fired");
      secondFlag = true;
      setDummyState({ a: "2" });
    }, 500);
  }

  console.log("reached second if-statement");
  if (!secondFlag) {
    console.log("rendering old content");
    return <div>old content</div>;
  }

  console.log("rendering new content");
  return <div>new content</div>;
}

Running this code prints on the console:

rendering starts... false false
ReproduceBug.js:10 reached first if-statement
ReproduceBug.js:20 reached second if-statement
ReproduceBug.js:22 rendering old content
ReproduceBug.js:7 rendering starts... true false
ReproduceBug.js:10 reached first if-statement
ReproduceBug.js:20 reached second if-statement
ReproduceBug.js:22 rendering old content
ReproduceBug.js:14 timeout fired
ReproduceBug.js:7 rendering starts... true true
ReproduceBug.js:10 reached first if-statement
ReproduceBug.js:20 reached second if-statement
ReproduceBug.js:26 rendering new content
ReproduceBug.js:7 rendering starts... true true
ReproduceBug.js:10 reached first if-statement
ReproduceBug.js:20 reached second if-statement
ReproduceBug.js:26 rendering new content

So it seems to me that the latest rendered JSX should contain the words "new content", yet the DOM still contains "old content".

The problem only occurs in strict mode.

Addendum: One might suggest that the mount-unmount-remount cycle in strict mode somehow causes this problem. While I don't argue against this being the case, I'd argue that it SHOULD NOT be the case: The 500ms timeout causes a re-render long after the remounting is finished, and does not carry over any data. The global flags do carry over data, but they are logged to prove that they do not confuse the rendering code. Yet, after the timeout, the rendering code runs again (twice), returns the new content, but the DOM stays at the old content. Even the expected behaviour of the remounting cycle in strict mode does not explain this behaviour, hence my assumption that this is a bug.

@MartinGeisse MartinGeisse added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label Apr 14, 2023
@MartinGeisse MartinGeisse changed the title Bug: Bug: old JSX from previous rendering visible in strict mode Apr 14, 2023
@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2023

It's not supported to setTimeout during rendering. Rendering is supposed to be pure and not have side effects. So from that point on, it doesn't make sense to try to reason about what the code is doing.

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2023

To clarify, since this might come across as overly defensive. I think there is sometimes an assumption that once you break the rules, it's okay to think about what the code does, because it should still behave in some meaningful way. So I wanted to convey my perspective that I work on the React team, and I usually give up as soon as I see some rule being broken. It's like there was a random variable in the middle of your calculation. At this point all my assumptions about how React works go out of the window because they don't matter.

Yes, I can try to retrace it, and explain what exactly goes wrong here. It would take some amount of effort. I can try to to do this. But ultimately a bug report needs to contain code that does not break the rules. Code that breaks the rules is never expected to work in any particular way, it might just happen to (not) work.

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2023

You're right this does look like a bug though.

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2023

Here's a minimal repro:

import { useState } from "react";

let ran = false;

export default function ReproduceBug() {
  const [state, setState] = useState("a");
  maybeRun(setState);
  return <div>{state}</div>;
}

function maybeRun(setState) {
  if (!ran) {
    ran = true;
    setTimeout(() => {
      setState("b");
    }, 500);
  }
}

What seems to happen here is that setState from the first StrictMode render is not capable of scheduling the update for some reason. So calls to it are ignored. However, in order to call it at all, you have to be breaking the rules of React (like in this repro, we're doing a side effect during rendering). If we were following the rules of React and either calling setState from an event handler or from an effect (or even during rendering directly — no timeouts), then the code would work. Because event handlers and "effects" would see setState from the second StrictMode render (which actually gets used):

export default function ReproduceBug() {
  const [state, setState] = useState("a");
  maybeRun(setState);
  return <div onClick={() => setState("c")}>{state}</div>;
}

Note clicking the div "works" because it "sees" the setState from the second StrictMode render (which got committed). You can verify that the setState versions between two StrictMode renders are indeed different:

import { useState } from "react";

export default function ReproduceBug() {
  const [state, setState] = useState("a");
  maybeRun(setState);
  return <div>{state}</div>;
}

let _setState;
function maybeRun(setState) {
  if (_setState && _setState !== setState) {
    throw Error("wat");
  }
  _setState = setState;
  setTimeout(() => {
    setState("b");
  }, 500);
}

I'm not sure why (maybe this is a bug, maybe this is a known difference), but as long as you follow the rules of React, I think this should not be observable. Regardless, this does not seem to be happening with @next versions. Maybe this means this was a bug that got fixed, or maybe it's a change to internal behavior. Since it breaks the rules in the first place, I'd say it's in the realm of undefined behavior.

@gaearon
Copy link
Collaborator

gaearon commented Apr 14, 2023

I've bisected the behavior change to #25583. Seems like this was an intentional change done for a different reason, but with fixing this as a consequence. This test in particular seems like our case: https:/facebook/react/pull/25583/files#diff-19df471970763c4790c2cc0811fd2726cc6a891b0e1d5dedbf6d0599240c127aR456-R482

So I think we can close this as fixed. But we don't strictly consider this a bugfix because the behavior for breaking the rules is kind of undefined.

@gaearon gaearon closed this as completed Apr 14, 2023
@MartinGeisse
Copy link
Author

I have to agree in the sense that I cannot reproduce this problem without breaking the rules. Any further discussion would be out of scope for a bug ticket. Thank you very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants