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 nested Suspense boundaries in Simple Suspense Renderer #334

Conversation

dios-david
Copy link
Contributor

While I was working on a real-life application using lazy components (using lazy and Suspense) when I noticed that the rendered HTML sometimes contains [object Promise] instead of properly resolved component markup.

I did some debugging and I found out that this issue happens only when there are nested lazy components in the app (App renders ComponentA lazily, ComponentA renders ComponentB lazily), in this case ComponentB is rendered as [object Promise].

If I get it right, there is a test case for validating nested Suspense boundaries (Async renderToString > should render JSX with nested suspense boundary), but that test case does not contain nested Suspense boundaries, only a single Suspense boundary with multiple nested suspended components.

I think both use-cases are valid (having one Suspense boundary per suspended component, or only having one "top-level" Suspense boundary), so I kept the original test with a bit more explicit name, and added a new one for testing nested Suspense boundaries which replicates my original issue:

  1) Async renderToString
       should render JSX with nested suspense boundaries:

      AssertionError: expected '<ul><li>one</li>[object Promise]<li>t…' to equal '<ul><li>one</li><li>two</li><li>three…'
      + expected - actual

      -<ul><li>one</li>[object Promise]<li>three</li></ul>
      +<ul><li>one</li><li>two</li><li>three</li></ul>
      
      at Context.<anonymous> (test/compat/async.test.js:61:23)
      at processTicksAndRejections (node:internal/process/task_queues:95:5)

As a solution I opted into a while loop instead of implementing a recursive Promise.all, but I don't have strong opinions on this.

Copy link

changeset-bot bot commented Feb 19, 2024

⚠️ No Changeset found

Latest commit: b6aa2ff

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 merged commit c57b230 into preactjs:simple-suspense-renderer-2024 Feb 19, 2024
1 check passed
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 <chrismsauve@gmail.com>
Co-authored-by: David Dios <david.dios@loveholidays.com>
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.

2 participants