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: useMutableSource inside a suspending component doesn't install the subscription #18885

Closed
orestis opened this issue May 11, 2020 · 4 comments
Assignees
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@orestis
Copy link

orestis commented May 11, 2020

React version: 0.0.0-experimental-33c3af284

Description

When using useMutableSource, you have to be careful where your Suspense boundaries are.

function Suspicious() {
  let payload = useMutableSource(mutableSource, getSnapshot, subscribe);
  return (
    <div>
      <span>Suspicious</span>
      <pre>{payload.read()}</pre>
    </div>
  );
}

In this case, the Suspense boundary is outside of this component. This means that if the component suspends at initial render, it doesn't properly mount, and apparently the subscription isn't ever installed. It seems like it's going to make refactoring components across Suspense boundaries very tricky.

Steps To Reproduce

  1. In the attached code sandbox, press the New Payload button
    Link to code example: https://codesandbox.io/s/subscribe-inside-suspense-q1u4u?file=/src/App.js

The current behavior

The "Working" component has successfully installed the useMutableSource subscription and has been updated with the new value.
The "Suspicious" component is stuck at being suspended, and its subscription hasn't been installed.

The expected behavior

The "Suspicious" component should have installed the subscription even if it had been suspended.

@orestis orestis added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label May 11, 2020
@bvaughn bvaughn self-assigned this May 12, 2020
@bvaughn
Copy link
Contributor

bvaughn commented May 12, 2020

This does not look like a problem with useMutableSource to me, but a Suspense bug in your demo app.

The Suspicious component suspends during the mount render, so it does not get committed to the DOM. Instead, its fallback UI gets committed. This means that no side effects- including subscribing to sources- will be run for that component. This is expected behavior.

However, those side effects will eventually be run once it gets committed. However, in your Sandbox this will never happen because you never resolve the thrown Promise:

var appState = {
  date: now,
  payload: suspendingValue(new Promise((x, y) => {})),
  //payload: suspendingValue(timeout(now, 1000)),
  subscribers: {}
};

If you fix that, the code should run as expected:
https://codesandbox.io/s/subscribe-inside-suspense-v9wpw

Going to close this issue since it doesn't look like a bug on React's side.

@bvaughn bvaughn closed this as completed May 12, 2020
@orestis
Copy link
Author

orestis commented May 12, 2020

Thanks for taking the time to look into this. I know why this is working the way it works. I'm raising this issue to ask if this is expected behaviour (it seems so) and to clarify that this is a limitation that falls out the way Suspense and effects interact.

Imagine that the suspending value wasn't indefinite (as shown in the demo) but something that took too long (say 10 seconds). During those 10 seconds, even if the "observed" value changed, the "observing" component cannot re-render ever, even if potentially the new value would not suspend the component at all.

The workaround obviously is to be careful not to rely on effects on components that might Suspend -> make Suspending components only a function of their props. I just wanted to have a discussion about this.

@bvaughn
Copy link
Contributor

bvaughn commented May 12, 2020

I know why this is working the way it works.

Sorry. It was not clear to me that you knew of the bug in the Sandbox you linked to.

I agree the scenario you describe would be a limitation.

@orestis
Copy link
Author

orestis commented May 12, 2020

This may well be a documentation issue (which is expected at this point since Suspense-for-data-fetching is new and experimental, and Suspense-for-code-splitting is expected to resolve quite quickly).

I'm not an expert in the React lifecycles, but if this wasn't React I'd say that the lifecycle I would expect for useMutableSource would be constructor/deconstructor instead of mount/unmount. I'm not sure if this is possible or desirable though, esp. if multiple instances of components could be around concurrently.

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