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: Fix source snippet decorator for story functions with suspense #17915

Merged
merged 3 commits into from
Jun 30, 2022

Conversation

redbugz
Copy link
Contributor

@redbugz redbugz commented Apr 8, 2022

fixes #17839

In Storybook 6.4, if a story function triggers Suspense by throwing a Promise like some third party hooks do, the story will fail to render
because the useEffect is skipped the first time and then run the second time, resulting in the
"Rendered more hooks than during the previous render." error
This runs the story function after the useEffect hook so the same number of hooks are run whether it triggers suspense or not, and in keeping with the guidelines on not conditionally running hooks.

This is preventing us from upgrading to 6.4, so it would be nice if this were backported to the 6.4 release as well.

Example scenario:

export function MyStory() {
  const [data] = useSomeHook() // <-- this third party hook triggers Suspense and will throw a Promise

  return <MyComponent data={data} />
}

There probably aren't too many hooks triggering Suspense yet, but we use one, and I imagine it will increase as React 18 and concurrent mode become more common, and especially once Suspense for Data Fetching is generally released.

We could wrap every story that uses this third-party hook in our own Suspense, but that seems tedious and can't be done with a decorator (we already have a decorator that wraps all our stories in Suspense, but this is called outside of that scope)

function MyStoryInner() {
  const [data] = useSomeHook() // <-- this third party hook triggers Suspense and will throw a Promise

  return <MyComponent data={data} />
}

export function MyStory() {
  return <Suspense fallback="Loading ..."><MyStoryInner /></Suspense>
}

But that seems tedious and makes it harder to write clean stories.

Issue:

What I did

I moved the execution of the storyFn until after the useEffect so the useEffect will run even if storyFn throws a Promise.

I don't understand the scope and semantics of SNIPPET_RENDERED enough to know if the useEffect should skip emitting that event until the Suspense resolves, or if it would be better to wrap the StoryFn in a <Suspense /> tag and provide some kind of fallback UI. Right now it will emit SNIPPET_RENDERED with an empty string until the Suspense resolves, then it will submit it again on the second render with the correct story content.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

If your answer is yes to any of these, please make sure to include it in your PR.

If a story function triggers Suspense by throwing a promise liike
some third party hooks do, the story will fail to render
because the useEffect is skipped the first time and then
run the second time, resulting in the
"Rendered more hooks than during the previous render." error
This runs the story function after the useEffect hook so the same number of hooks
are run whether it triggers suspense or not, and in keeping
with the guidelines on not conditionally running hooks
@nx-cloud
Copy link

nx-cloud bot commented Apr 8, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit ba31493. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@shilman shilman changed the title handle suspense in story function React: Fix source snippet decorator for story functions with suspense Apr 8, 2022
@yannbf
Copy link
Member

yannbf commented Apr 11, 2022

Hey @redbugz thanks for making this PR! Seems like a delicate change, @ndelangen could you take a look at this? Thanks!

@ndelangen ndelangen merged commit 79ca046 into storybookjs:next Jun 30, 2022
@redbugz redbugz deleted the 17839-fix-story-suspense branch June 30, 2022 20:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error in jsxDecorator if a story function triggers Suspense
4 participants