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: SuspenseList crash #20932

Closed
gaearon opened this issue Mar 5, 2021 · 1 comment · Fixed by #20948
Closed

Bug: SuspenseList crash #20932

gaearon opened this issue Mar 5, 2021 · 1 comment · Fixed by #20948

Comments

@gaearon
Copy link
Collaborator

gaearon commented Mar 5, 2021

https://codesandbox.io/s/nostalgic-frost-53gys?file=/src/App.js

import * as React from "react";

function S(props) {
  return <React.Suspense {...props} />;
}

export default function App() {
  return (
    <React.unstable_SuspenseList revealOrder="forwards">
      <S fallback={null}>
        <Bad />
      </S>
      <S fallback={null}>
        <h1>wow</h1>
      </S>
    </React.unstable_SuspenseList>
  );
}

function Bad() {
  const [c, setC] = React.useState(0);
  read(c);
  const r = React.useRef(true);
  React.useEffect(() => {
    if (r.current) {
      r.current = false;
      setC((x) => x + 1);
    }
  }, [c]);
  return <h2>wow</h2>;
}

const cache = {};
function read(i) {
  if (cache[i]) {
    if (cache[i].result) {
      return cache[i].result;
    } else {
      throw cache[i].promise;
    }
  }
  cache[i] = {
    promise: new Promise((resolve) => {
      setTimeout(() => {
        cache[i].result = "foo";
        resolve();
      }, 1000);
    })
  };
  throw cache[i].promise;
}
@gaearon gaearon added Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Component: Concurrent Features Type: Bug and removed Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Mar 5, 2021
@milobluebell
Copy link

milobluebell commented Mar 5, 2021

i think it's because , you had thrown a promise object in read function,

function read(i) {
  if (cache[i]) {
    if (cache[i].result) {
      return cache[i].result;
    } else {
      // *** HERE ****
      throw cache[i].promise;
    }
  }
  cache[i] = {
    promise: new Promise((resolve) => {
      setTimeout(() => {
        cache[i].result = "foo";
        resolve();
      }, 1000);
    })
  };
  // *** and HERE ****
  throw cache[i].promise;
}

it interrupt your ideal rendering. and the error thrown by React in browser means that there isn't an fiberRoot could be founded

acdlite added a commit to acdlite/react that referenced this issue Mar 5, 2021
Based on facebook#20932

Co-Authored-By: Dan Abramov <dan.abramov@gmail.com>
acdlite added a commit that referenced this issue Mar 7, 2021
* Add failing regression test

Based on #20932

Co-Authored-By: Dan Abramov <dan.abramov@gmail.com>

* Reset `subtreeFlags` in `resetWorkInProgress`

Alternate fix to #20942

There was already a TODO to make this change, but at the time I left it,
I couldn't think of a way that it would actually cause a bug, and I was
hesistant to change something without fully understanding the
ramifications. This was during a time when we were hunting down a
different bug, so we were especially risk averse.

What I should have done in retrospect is put the change behind a flag
and tried rolling it out once the other bug had been flushed out.

OTOH, now we have a regression test, which wouldn't have otherwise, and
the bug it caused rarely fired in production.

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
koto pushed a commit to koto/react that referenced this issue Jun 15, 2021
* Add failing regression test

Based on facebook#20932

Co-Authored-By: Dan Abramov <dan.abramov@gmail.com>

* Reset `subtreeFlags` in `resetWorkInProgress`

Alternate fix to facebook#20942

There was already a TODO to make this change, but at the time I left it,
I couldn't think of a way that it would actually cause a bug, and I was
hesistant to change something without fully understanding the
ramifications. This was during a time when we were hunting down a
different bug, so we were especially risk averse.

What I should have done in retrospect is put the change behind a flag
and tried rolling it out once the other bug had been flushed out.

OTOH, now we have a regression test, which wouldn't have otherwise, and
the bug it caused rarely fired in production.

Co-authored-by: Dan Abramov <dan.abramov@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants