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

Suspense layout effects: Failing test case #21853

Closed
wants to merge 1 commit into from

Conversation

bvaughn
Copy link
Contributor

@bvaughn bvaughn commented Jul 12, 2021

The key components to this are:

  1. State updater function as a ref setter.
  2. Render-phase update, e.g. mirrored props-to-state (not a generally recommended patter).
  3. Suspend in an update (also not a common or recommended pattern).

This causes a cycle:

  1. Ref gets set to null by React, which schedules a state update (because ref is useState)
  2. Component schedules more work during the render phase (b'c of the mirrored value in state)
  3. Repeat...

The reason enableSuspenseLayoutEffectSemantics is related to this issue is because React doesn't otherwise reset refs to null when an update suspends. (Note that React does eventually set the ref to null when the component unmounts, but since the component doesn't render again after, there's no render-phase update to cycle.)

@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Jul 12, 2021
@sizebot

This comment has been minimized.

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 12, 2021

I think this same scenario can be reproduced without the enableSuspenseLayoutEffectSemantics so long as conditional rendering is used. If an update both causes one component to suspend and causes an element with a state-updater-ref to be unmounted, the same bug will still occur.

I was mistaken here. Without the flag being enabled, the suspending update doesn't get committed until the promise resolves, so the ref still gets unmounted, but later. With the GK enabled, it's done right away, as part of committing the suspending update.

@gaearon
Copy link
Collaborator

gaearon commented Jul 12, 2021

Here's a test that uses a more "legit" pattern and also fails.

  fit('should not cause a cycle when combined with a render phase update #2', () => {
    let scheduleSuspendingUpdate;

    function App() {
      const [value, setValue] = React.useState(true);

      scheduleSuspendingUpdate = () => setValue(!value);

      return (
        <>
          <React.Suspense fallback="Loading...">
            <ComponentThatCausesBug value={value} />
            <ComponentThatSuspendsOnUpdate shouldSuspend={!value} />
          </React.Suspense>
        </>
      );
    }

    function ComponentThatCausesBug({value}) {
      const [rect, setRect] = React.useState(null);

      const measure = React.useCallback((node) => {
        if (node) {
          setRect(node.getBoundingClientRect());
        } else {
          setRect(null);
        }
      }, []);

      return <div ref={measure} />;
    }

    const promise = Promise.resolve();

    function ComponentThatSuspendsOnUpdate({shouldSuspend}) {
      if (shouldSuspend) {
        // Fake Suspend
        throw promise;
      }
      return null;
    }

    TestUtils.act(() => {
      const root = ReactDOM.createRoot(document.createElement('div'));
      root.render(<App />);
    });

    TestUtils.act(() => {
      scheduleSuspendingUpdate();
    });
  });

@bvaughn
Copy link
Contributor Author

bvaughn commented Jul 12, 2021

These two code snippets should be functionally equivalent:

const ref = useRef(null);
const [rect, setRect] = useState(null);

useEffect(() => {
  const div = ref.current;
  setRect(div.getBoundingClientRect());

  return () => {
    // Dunno why you'd do this but maybe there's a valid reason
    setRect(null);
  };
}, []);
const [rect, setRect] = React.useState(null);

const refCallback = React.useCallback(ref => {
  if (ref) {
    setRect(ref.getBoundingClientRect());
  } else {
    // Dunno why you'd do this but maybe there's a valid reason
    setRect(null);
  }
}, []);

But the second one might break with the new feature flag enabled (if an update suspends and if there's a render phase update as well). I think this is a React bug.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants