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

React.Suspense provide a lifecycle so components can handle the `display:none` removal #14536

Open
oliviertassinari opened this issue Jan 5, 2019 · 23 comments

Comments

@oliviertassinari
Copy link
Contributor

@oliviertassinari oliviertassinari commented Jan 5, 2019

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

It's a feature.

What is the current behavior?

React.Suspense mounts its children with a display: none style if a promise is thrown. Once the thrown promise is resolved, React removes the display: none style.

What is the expected behavior?

The children components have no easy way to know when the display: none style is removed by React. This is problematic when one child component needs to read from the DOM layout to correctly display its elements. Most people wait for the componentDidMount callback to trigger, but because the element is display: none, it can't read any value from the DOM layout.

The issue was discovered in mui-org/material-ui#14077. I believe that React should provide a lifecycle so the children components know when they are visible, that it's safe to do layout computations.

The best workaround I'm aware of it to use the Intersection Observer API but it requires a polyfill on IE 11 and Safari.

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

Version: 16.7.0-alpha.2

@ryancogswell

This comment has been minimized.

Copy link

@ryancogswell ryancogswell commented Feb 18, 2019

A new lifecycle doesn't feel like the right solution to me. In hooks implementations, the logic typically involved would be in useLayoutEffect and it seems that it would get convoluted to need to interact with a separate lifecycle for this Suspense scenario (brings back the kind of bugs that people tend to introduce with the separation of componentDidMount/Update/Unmount).

It seems that this really needs to be treated more like state of the Suspense element -- especially since it can toggle any number of times if anything causes a descendant to re-render in a way that now loads a lazy-loaded component (and of course many more scenarios once Suspense is supported for fetching, etc.). Then the appropriate way for descendants to use this information would be for it to be provided via some sort of Context. Perhaps there could be something like a useSuspenseContext hook that would return whether or not the element is currently contained within a Suspense that is in its fallback state. This boolean could then be leveraged in the useLayoutEffect dependency array. Alternatively, something like a useSuspenseAwareLayoutEffect could automatically execute whenever this boolean changes and/or provide the boolean as an argument to the effect function.

@ryancogswell

This comment has been minimized.

Copy link

@ryancogswell ryancogswell commented Feb 19, 2019

Perhaps someone on the React team has already put a lot of thought into this and knows what they plan to do and I'm just wasting brain cycles on it, but I want to follow-up on my earlier thoughts after some more consideration.

I like the idea of using a context type built into React (e.g. React.SuspendedContext). This avoids needing a new hook or lifecycle to get at this information. Since this likely needs to be taken into account by any component that uses useLayoutEffect to get the dimensions of itself or one of its parts in the DOM, it seems like there should be a mechanism built directly into useLayoutEffect to get this information. I propose for consideration the approaches in the following two examples. The intent would be for these two examples to behave identically (though in the second, it would be possible to avoid a re-render and execute only the effect when suspended changes).

const MyComponent = props => {
   const suspended = useContext(React.SuspendedContext);
   useLayoutEffect(() => {
      if (!suspended) {
         // measure stuff in DOM and do something with that info
      }
   }, [suspended]);
   return <Stuff/>;
};
const MyComponent = props => {
   // use arity of effect function as a shorthand indicator of subscribing to React.SuspendedContext
   // as a dependency for this effect and provide the value as the effect function argument.
   useLayoutEffect((suspended) => {
      if (!suspended) {
         // measure stuff in DOM and do something with that info
      }
   }, []);
   return <Stuff/>;
};

I would actually prefer for the boolean to be the opposite of suspended since you would generally only care about doing something in the "not suspended" case, but I was having a hard time coming up with a variable name for the opposite that relates clearly to Suspense and that doesn't imply something that may not be true (e.g. visible isn't necessarily true if the component is hidden for reasons other than Suspense).

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

Can you please provide a minimal reproducing example? Without Material UI etc. Just React.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

(For future bug reports, that's generally a good idea since it lets avoid stalling on the issue when we're finally ready to look at it :-)

@rovansteen

This comment has been minimized.

Copy link

@rovansteen rovansteen commented Mar 5, 2019

What’s there to reproduce here? It’s not really a bug, but a feature request.

If you want to animate a component for example there’s no way to know when it’s actually mounted and visible to the user to start the animation.

If there’s a bug here ignore what I said, but in that case I’m wondering how this problem (in theory) would be solved with Suspense.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

A code sandbox demonstrating the problem you're running into (which necessitates the feature request) is what I'm asking for.

@ryancogswell

This comment has been minimized.

Copy link

@ryancogswell ryancogswell commented Mar 5, 2019

@gaearon The code sandbox I provided in #14869 (https://codesandbox.io/s/40kw74nyk9) reproduces the problem without any other dependencies.

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Mar 5, 2019

I think this is only ever a problem if the component that didn't suspend tries to read its size. Since the Suspense adds a parent DOM node with display: none the dimensions are 0 (or rather unknown).

I guess this was always a bug if you used these components inside a node that was initially display: none;. However, before at least one component in the tree knew about the change (and could implement some API to read/notify others about the change). Now react does this behind the curtain without giving us some API to check if the parent switched from display: none to display: not-none;.

Edit: Another codesandbox: https://codesandbox.io/s/qx4mv0q2vw. This is a bit constructed. For an actual example the material-ui tabs can be used. They display scroll buttons depending on scrollLeft or scrollRight.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

Thanks @ryancogswell.

I think the problem is just that we unhide the parent node after inserting the children. If the order of DOM operations was swapped I think this would have worked. Does this sound right?

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Mar 5, 2019

Thanks @ryancogswell.

I think the problem is just that we unhide the parent node after inserting the children. If the order of DOM operations was swapped I think this would have worked. Does this sound right?

Aren't the components that didn't suspend currently mounted immediately? In other words: Let A and B be children of Suspense. A suspends, B doesn't: B is immediately mounted but wrapped inside <div style="display: none;" />. Are you saying you want to defer mounting of every child until none is suspended anymore?

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

Aren't the components that didn't suspend currently mounted immediately?

No. React never mounts trees in an inconsistent state.

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Mar 5, 2019

I think we're not talking about the same thing:

<Suspense>
  <SomethingThatSuspends />
  <SomethingThatMountsImmediatelyAndNeedsLayout />
</Suspense>

or https://codesandbox.io/s/8894qpr3w2

@sebmarkbage

This comment has been minimized.

Copy link
Member

@sebmarkbage sebmarkbage commented Mar 5, 2019

No. React never mounts trees in an inconsistent state.

Actually, we do in sync mode.

@eps1lon

This comment has been minimized.

Copy link
Contributor

@eps1lon eps1lon commented Mar 5, 2019

No. React never mounts trees in an inconsistent state.

Actually, we do in sync mode.

That actually fixes it. If I put everything in unstable_ConcurrentMode it works as exepcted: https://codesandbox.io/s/8894qpr3w2. In sync mode it displays "We have 0px"; in concurrent it displays "We have 732px" (depending on screen size).

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Mar 5, 2019

My bad.

@ryancogswell

This comment has been minimized.

Copy link

@ryancogswell ryancogswell commented Mar 5, 2019

@sebmarkbage So in Concurrent Mode, if children within a Suspense have already been rendered and mounted and a state change occurs that causes a new lazy child to be introduced, will the existing children be unmounted and then remounted once the lazy child is loaded?

If so, then it sounds like the behavior in Concurrent Mode would take care of this completely. In the React 16 Roadmap it indicates that Concurrent Mode will be opt-in. Is that just for React 16? Would it likely become the default for React 17? I'm just trying to figure out whether there is any need for an enhancement to address this other than the release of Concurrent Mode. If Concurrent Mode is going to remain opt-in beyond the next major version, then there may still be a need for an enhancement to address this issue when not using Concurrent Mode.

It may also be sufficient to just add documentation for Suspense that warns about this specific limitation when using it without Concurrent Mode. It really depends on how you intend to support the "full" set of Suspense features (e.g. data fetching). Will the full Suspense features be available with and without Concurrent Mode (just with some differences in behavior)? Or will you have to opt-in to Concurrent Mode to use them?

@sebmarkbage

This comment has been minimized.

Copy link
Member

@sebmarkbage sebmarkbage commented Mar 6, 2019

@ryancogswell For an update in concurrent mode, the existing children will be hidden without issuing any life-cycles and then once they unsuspend, they will be unhidden before the life-cycles fire.

Adopting Concurrent Mode will be hard for a lot of people so it will remain opt-in for quite a while.

@ryancogswell

This comment has been minimized.

Copy link

@ryancogswell ryancogswell commented Mar 6, 2019

I thought I was going to be able to still expose this as an issue in the update case in concurrent mode, but the timing of the effects in concurrent mode allows everything to work wonderfully! 🥇

Here's my modified sandbox using sync on the top and concurrent below with a "Toggle Header Size" button that demonstrates the issue for an update. Both the initial render and update demonstrate the issue in sync mode. Both work great in concurrent mode.

Given that concurrent mode fully handles this, I don't think any separate enhancement is necessary, but given that many people won't be using concurrent mode any time soon, the documentation for React.lazy/Suspense should probably warn about this limitation when not using concurrent mode.

@gaearon

This comment has been minimized.

Copy link
Member

@gaearon gaearon commented Apr 29, 2019

It seems like the fix to this will be #15504. Note it requires that your tree is Strict Mode compliant. We can't fix this in legacy mode.

@oliviertassinari

This comment has been minimized.

Copy link
Contributor Author

@oliviertassinari oliviertassinari commented Apr 29, 2019

I will be happy to give it a try once it's available for test. I will see if it fixes the bug reported on Material-UI side 😍.

@zioth

This comment has been minimized.

Copy link

@zioth zioth commented Sep 27, 2019

I think this is a bug. In non-lazy components, you can depend on render() producing something visible before componentDidMount is called. In lazy components, that's not true. This means components have to be aware of something happening outside of them (whether they were lazy-loaded). I'm not a fan of fixing what looks to be a bug, with an opt-in, unstable feature like unstable_createSyncRoot.

What makes this worse is that componentDidUpdate is not called when display:none is removed, so there's really know way to know when the element becomes measurable.

@More4ever

This comment has been minimized.

Copy link

@More4ever More4ever commented Oct 2, 2019

Any update here?

unstable_createSyncRoot or unstable_createRoot fix issue with suspense direct styles manipulation, but break hot module replacement (page content adds to previous each update)

Maybe it worth prevent Suspense child renders in progress of async request (using some props for Suspense component)

@chrisvasz

This comment has been minimized.

Copy link

@chrisvasz chrisvasz commented Nov 6, 2019

Here is another use case that is really difficult right now:

I'm building a chat UI that uses some lazy components in the "thread" view. I want the thread view to open to the most recent message at the bottom, but without an event to tell me when the thread is no longer suspended, I'm having to hack an event together with a MutationObserver. Is there a better way to do this?

function App() {
  ...
  return (
    <Suspense>
      <Thread />
    </Suspense>
  );
}

function Thread() {
  let ref = useRef(null);
  const scrollToBottom = useCallback(() => {
    let current = ref.current;
    if (current) current.scrollTop = current.scrollHeight;
  }, []);

  // Since the children of this component may or may not suspend, I have to handle both cases
  useLayoutEffect(scrollToBottom, []); // children don't suspend
  useCallbackOnUnsuspendEffect(ref, scrollToBottom); // children do suspend

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

function useCallbackOnUnsuspendEffect(ref, callback) {
  return React.useEffect(() => {
    let node = ref.current;
    if (node == null) return;
    const observer = new global.MutationObserver(mutations => {
      if (mutations.find(m => m.attributeName === 'style')) {
        callback();
      }
    });
    observer.observe(node, { attributes: true });
    return () => observer.disconnect();
  }, [callback, ref]);
}

This implementation is brittle for multiple reasons:

  • it relies on an undocumented internal behavior of suspense (modifying the style attribute of a DOM node)
  • it relies on a specific component structure between the Suspense and the DOM node that gets observed by the mutation observer. If I add a component that renders DOM between the Thread and the Suspense boundary, then the display: none will be applied to the top-most rendered DOM node and my MutationObserver misses the events.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.