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

showPreparingDocs applies display: none and gets into a race with React 18's rendering cycle #18347

Closed
snowystinger opened this issue May 27, 2022 · 4 comments
Assignees
Labels

Comments

@snowystinger
Copy link
Contributor

snowystinger commented May 27, 2022

Describe the bug
Storybook's preparing docs gets into a race with React 18's rendering cycle. This can lead to bounding client rects being all zeros and cause flake in tests that need to measure during the useLayoutEffect phase.
There is also a second effect, the document.body's userAgent stylesheet margin of 8px is also not removed early enough. So not only are measured values 0, but positioning calculations become off by 8px.
@tmeasday

To Reproduce
When running a story how Chromatic does, for example
https://5f0dd5ad2b5fc10022a2e320-ugizaypgfl.chromatic.com/iframe.html
and then opening the js console and running

__STORYBOOK_ADDONS_CHANNEL__.emit('setCurrentStory', { storyId: 'menutrigger--direction-start-end' });

You can see that the overlay position is wrong (Happens to coworkers with Intel based Mac's, happens in chromatic, does not seem to happen to ARM based Mac's).

I made a smaller reproduction using CRA + npx storybook init here:
https://github.com/snowystinger/storybook-timing

npm i
npm run storybook

go to localhost:6006/iframe.html

__STORYBOOK_ADDONS_CHANNEL__.emit('setCurrentStory', { storyId: 'example--demo' });

There is one thing that was necessary to push it into the race condition reliably, which was to add the same typekit script we have. This is a blocking script, so I suspect you could accomplish the same race on any machine by adding a blocking script of variable time length. It's in .storybook/preview-head.html https://github.com/snowystinger/storybook-timing/blob/main/.storybook/preview-head.html

System
Machine that reproduces:

  System:
    OS: macOS 12.3.1
    CPU: (16) x64 Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz
  Binaries:
    Node: 16.14.0 - ~/.nvm/versions/node/v16.14.0/bin/node
    Yarn: 1.22.18 - ~/.nvm/versions/node/v16.14.0/bin/yarn
    npm: 8.3.1 - ~/.nvm/versions/node/v16.14.0/bin/npm
  Browsers:
    Chrome: 101.0.4951.64
  npmPackages:
    @storybook/addon-actions: ^6.5.5 => 6.5.5 
    @storybook/addon-essentials: ^6.5.5 => 6.5.5 
    @storybook/addon-interactions: ^6.5.5 => 6.5.5 
    @storybook/addon-links: ^6.5.5 => 6.5.5 
    @storybook/builder-webpack5: ^6.5.5 => 6.5.5 
    @storybook/manager-webpack5: ^6.5.5 => 6.5.5 
    @storybook/node-logger: ^6.5.5 => 6.5.5 
    @storybook/preset-create-react-app: ^4.1.1 => 4.1.1 
    @storybook/react: ^6.5.5 => 6.5.5 
    @storybook/testing-library: ^0.0.11 => 0.0.11 

Machine that does NOT reproduce:

  System:
    OS: macOS 12.3.1
    CPU: (10) arm64 Apple M1 Max
  Binaries:
    Node: 16.13.2 - ~/.nvm/versions/node/v16.13.2/bin/node
    Yarn: 1.22.17 - /opt/homebrew/bin/yarn
    npm: 8.1.2 - ~/.nvm/versions/node/v16.13.2/bin/npm
  Browsers:
    Chrome: 101.0.4951.64
  npmPackages:
    @storybook/addon-actions: ^6.5.5 => 6.5.5 
    @storybook/addon-essentials: ^6.5.5 => 6.5.5 
    @storybook/addon-interactions: ^6.5.5 => 6.5.5 
    @storybook/addon-links: ^6.5.5 => 6.5.5 
    @storybook/builder-webpack5: ^6.5.5 => 6.5.5 
    @storybook/manager-webpack5: ^6.5.5 => 6.5.5 
    @storybook/node-logger: ^6.5.5 => 6.5.5 
    @storybook/preset-create-react-app: ^4.1.1 => 4.1.1 
    @storybook/react: ^6.5.5 => 6.5.5 
    @storybook/testing-library: ^0.0.11 => 0.0.11 

Additional context
Here's our workaround for the time being
https://github.com/adobe/react-spectrum/pull/3165/files

We think this the problem area https://github.com/storybookjs/storybook/blob/main/lib/preview-web/src/WebView.ts#L168

Note, we don't use docs in Chromatic and I've turned it off in the example repo.

@tmeasday
Copy link
Member

tmeasday commented May 31, 2022

Hi @snowystinger. Thanks for the reproduction.

A couple of observations, right off the bat.

  1. I am on an ARM mac. I see the original bug, but the reproduction isn't reproducing here (I assume the issue is the bounding box logged in useLayoutEffect is different to the one logged in useEffect?)

  2. It is super weird that showPreparingDocs is getting called at all, given this is a story. I will dig into why this is happening. However, I might have guessed that showPreparingStory should have been called, which has similar behaviour to showPreparingDocs, so I am not sure if fixing that will resolve the issue.

  3. I put in log points on showPreparingDocs, showPreparingStory and showMode in the reproduction and I see:

image

(Note it doesn't reproduce here). Would be interesting to see what's logged in the case it does reproduce.

  1. I did the same in the original issue (which does reproduce for me), and saw this:

image

I think what's happening here is simply that the JS bundle that includes the story (https://5f0dd5ad2b5fc10022a2e320-ugizaypgfl.chromatic.com/5.1f4938b8.iframe.bundle.js in case 4. above) takes longer than 100ms to parse + execute. So the spinner appears very briefly and the story renders behind it.

In story view mode we do take steps to ensure that the #root element where the story renders is not display: none during rendering, so thinking it through, it is possible the behaviour will be differently if the story spinner is displayed instead of the docs one.

@shilman
Copy link
Member

shilman commented Jun 3, 2022

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.7-alpha.0 containing PR #18370 that references this issue. Upgrade today to the @prerelease NPM tag to try it out!

npx sb upgrade --prerelease

Closing this issue. Please re-open if you think there's still more to do.

@shilman shilman closed this as completed Jun 3, 2022
@shilman
Copy link
Member

shilman commented Jun 6, 2022

Zoinks!! I just released https://github.com/storybookjs/storybook/releases/tag/v6.5.7 containing PR #18370 that references this issue. Upgrade today to the @latest NPM tag to try it out!

npx sb upgrade

@shilman
Copy link
Member

shilman commented Jun 6, 2022

Ermahgerd!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-alpha.0 containing PR #18370 that references this issue. Upgrade today to the @future NPM tag to try it out!

npx sb upgrade --prerelease

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants