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

Fixing rendering multiple suspended components within one Suspense boundary #335

Conversation

dios-david
Copy link
Contributor

Rendering a Suspense boundary which have multiple suspended components as children fails:

const promise = renderToStringAsync(
  <ul>
    <Suspense fallback={null}>
      <SuspenderOne>
        <li>one</li>
      </SuspenderOne>
      <SuspenderTwo>
        <li>two</li>
      </SuspenderTwo>
    </Suspense>
  </ul>
);
  1) Async renderToString
       should render JSX with multiple suspended direct children within a single suspense boundary:
     Error: the promise {} was thrown, throw an Error :)

I was curious so I trying to debug and fix this, what I found is:

  • _renderToString (with asyncMode: true) throws a Promise while rendering the children of a component, if any of the children is a suspended component
  • in this case the error Promise is caught, and once it resolves the same render is called again (without the try-catch this time)
  • but when that second render throws again (e.g. when another suspended child component is still not resolved), that Promise is not caught

I changed this a bit by moving the "render children, if that throws a Promise then render again once that Promise is resolved" bit to a function and re-using that whenever rendering a children. This way if a Suspense component has multiple suspended child components, we keep rendering until all of the promises are resolved.

Copy link

changeset-bot bot commented Feb 19, 2024

⚠️ No Changeset found

Latest commit: 0fc7a17

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@JoviDeCroock JoviDeCroock marked this pull request as ready for review February 19, 2024 17:57
@JoviDeCroock JoviDeCroock merged commit cf81587 into preactjs:simple-suspense-renderer-2024 Feb 20, 2024
1 check passed
Comment on lines +441 to +453
const renderChildren = () =>
_renderToString(
rendered,
context,
isSvgMode,
selectValue,
vnode,
asyncMode
);

try {
// Recurse into children before invoking the after-diff hook
const str = renderChildren();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Worth benchmarking in the future. Creating inline closures is likely slower than inlining it into the various places.

JoviDeCroock added a commit that referenced this pull request Feb 20, 2024
* Simple Suspense renderer

* update simple suspense rendere

* add a possible promise string as the return value

* Update test/compat/async.test.js

* Create pink-gifts-kneel.md

* non breaking

* Update async.test.js

* fixing nested Suspense boundaries (#334)

* fixing multiple suspended child components (#335)

---------

Co-authored-by: Chris Sauve <[email protected]>
Co-authored-by: David Dios <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants