Build fails when error thrown inside Suspense boundary

[jabrks]

Preliminary Checks

• This issue is not a duplicate. Before opening a new issue, please search existing issues: https://github.com/gatsbyjs/gatsby/issues
• This issue is not a question, feature request, RFC, or anything other than a bug report directly related to Gatsby. Please post those things in GitHub Discussions: https://github.com/gatsbyjs/gatsby/discussions

Description

I am trying to use the following pattern from the React docs to opt out of server rendering for a particular component

<Suspense fallback={<Loading />}>
  <Chat />
</Suspense>

function Chat() {
  if (typeof window === 'undefined') {
    throw Error('Chat should only render on the client.');
  }
  // ...
}

However, Gatsby does not appear to support this and instead fails whilst attempting to build static HTML for the page in question

Reproduction Link

https://codesandbox.io/p/sandbox/gatsby-ssr-c2foq1

Steps to Reproduce

Run gatsby build

Expected Result

The build should complete successfully

Actual Result

The build fails

Environment

System:
  OS: macOS 13.2
  CPU: (10) arm64 Apple M1 Pro
  Shell: 5.8.1 - /bin/zsh
Binaries:
  Node: 16.19.0 - ~/.nvm/versions/node/v16.19.0/bin/node
  Yarn: 1.22.19 - ~/.nvm/versions/node/v16.19.0/bin/yarn
  npm: 8.19.3 - ~/.nvm/versions/node/v16.19.0/bin/npm
Browsers:
  Chrome: 109.0.5414.119
  Firefox: 109.0
  Safari: 16.3

Config Flags

No response

[skan654]

Hi.

I was wondering if you might have a spot on your team to help resolve the issue at hand.

[pieh]

You are right. Our current implementation crashes the build on any error anywhere (which includes tree under <Suspense> currently).

In particular

gatsby/packages/gatsby/cache-dir/static-entry.js

Lines 340 to 347 in bc6c3ef

	const { pipe } = renderToPipeableStream(bodyComponent, {
	onAllReady() {
	pipe(writableStream)
	},
	onError(error) {
	writableStream.destroy(error)
	},
	})

fails the build in onError.

Per React docs ( https://beta.reactjs.org/reference/react-dom/server/renderToPipeableStream ) we should fail the build only in onShellError and let onError ones to be non fatal, but probably still displayed (which might make output log quite verbose if you use such pattern on a lot of pages)

[pieh]

I started work on fixing this to match React guideline in #37648

[pieh]

Some news here. I've was able to adjust html gen to not fail in my PR, but what I'm seeing right now is the setup with suspended components throwing in production builds result in this error https://reactjs.org/docs/error-decoder.html/?invariant=419 in the browser being logged and development showing error overlay:

I did check wether Next.js have similar behavior and it actually does as well:

So ... it doesn't seem like this way of skipping components in SSR actually does work? Are you aware of any framework or other reference where this setup actually work in practice?

[jabrks]

This was the first time I had tried implementing it, but I can replicate the behaviour you're seeing in Next.js as well. I also tried with Remix, which doesn't trigger an overlay but still throws an error in the browser console. The original React documentation for the pattern doesn't mention this, so it's unclear to me whether it's expected or not

[MRRaduan]

Does this have some relation with the problem mentioned in #36678, specifically here?

sometimes I have to restart the server, sometimes it works. It seems that the issue is when you have a Suspence and lazy loaded component that is not hot reloaded correctly. I'm not working on the Gatsby project right now but I'll try to make some specific tests to understand if there are any patterns when HMR is not working

[gaearon]

You'd need to also implement onRecoverableError for hydrateRoot and suppress logging that particular error.

Here's how Next.js does this for its own "noSSR" feature:

https://github.com/vercel/next.js/blob/cd0ebd8e8cf922360a6d3150b29c90013cd92d48/packages/next/client/app-index.tsx#L121-L134

https://github.com/vercel/next.js/blob/1ce3b3617901dc037ee5628d7ecfc104ade7e5e5/packages/next/src/shared/lib/lazy-dynamic/dynamic-no-ssr.tsx#L7-L9

So Gatsby could expose something similar.

[lezan]

Hey @gaearon thanks for the feedback. Also as pointed by @MRRaduan, could this one related to #36678?

[LekoArts]

@gaearon You've said that Next.js implemented what you mentioned but as shown in #37613 (comment) it also doesn't work there?

[jabrks]

I think Dan was referring to the ssr option documented here: https://nextjs.org/docs/advanced-features/dynamic-import#with-no-ssr