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

Stories disappear after HMR #7493

Closed
city41 opened this issue Jul 19, 2019 · 23 comments
Closed

Stories disappear after HMR #7493

city41 opened this issue Jul 19, 2019 · 23 comments

Comments

@city41
Copy link

city41 commented Jul 19, 2019

Describe the bug
While Storybook is running, after making a change to source and saving, webpack rebuilds and HMR updates the browser. But when this happens, many of the stories disappear.

To Reproduce
Steps to reproduce the behavior:

  1. Launch storybook
  2. After it has built and launched in the browser, make a code change and save
  3. wait for HMR to finish

Expected behavior
All the stories to remain, and the new code changes to be present.

Actual behavior: most stories disappear

Screenshots
Starting state:

image

After performing the above steps:

image

System:

  • OS: Ubuntu 18.04
  • Device: Lenovo ThinkPad
  • Browser: Chrome 75.0.3770.100
  • Framework: React 16.8
  • Addons: addon-docs
  • Version: 5.2.0.-beta.3

Additional context
I have been using the 5.2.0 alphas (and now betas) to try out the docs features. My stories are written in MDX. I did not notice this happening when I was using the alphas, but it does occur for me with both 5.2.0-beta.0 and 5.2.0-beta.3

The only error I see in the browser console is vendors~main.44efb48ad30cf85163a3.bundle.js:585 The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type". which doesn't seem relevant.

In the terminal, I see no errors. I do get warnings like this

WARNING in ./packages/react-primitives/src/components/button/index.ts 1:0-68
"export 'PrimaryButtonProps' was not found in './primaryButton'
 @ ./packages/react-primitives/src/components/button/__stories__/primaryButton.stories.mdx
 @ ./packages sync .stories.mdx$
 @ ./.storybook/config.js
 @ multi ./node_modules/@storybook/core/dist/server/common/polyfills.js ./node_modules/@storybook/core/dist/server/preview/globals.js ./.storybook/config.js ./node_modules/webpack-hot-middleware/client.js?reload=true&quiet=true

These warnings are all for TypeScript interfaces. On the surface they seem unrelated (I've always gotten these warnings)

If I refresh the browser, the stories all return to normal.

@shilman
Copy link
Member

shilman commented Jul 20, 2019

@city41 Sounds pretty weird. I just tested it out in a simple project and wasn't able to reproduce. I did find a corner case bug in the process #7497, but don't think it's related to what you're seeing.

Are there any patterns to which stories are disappearing? Are entire files not loading, or are some stories from files partially loading and others not loading?

Here's what I'd suggest:

  1. If you're up for it, check out the "How do I debug my MDX story?" in the tech preview doc. Mostly those steps give transparency into what's happening under the hood.

  2. If you can't figure it out, please create a repro repo and I'd be happy to take a look. Or if you're on the Storybook discord we can debug it together.

Thanks!!

@city41
Copy link
Author

city41 commented Jul 22, 2019

I am going to dig into this today and see if I can contribute anything helpful.

@city41
Copy link
Author

city41 commented Jul 22, 2019

From what I can tell, this issue was introduced in 5.2.0-alpha.37. When I use 5.2.0-alpha.36 or any previous alphas, the stories never disappear. From 5.20-alpha.37 on up to the most recent beta, stories disappear after an HMR load.

I didn't try every single release :) I binary searched to find 37.

I'll keep digging into 37's changes and see if I can find a culprit.

@shilman
Copy link
Member

shilman commented Jul 22, 2019

It's probably related to this change: #7319

I also just made a related change & bugfix in this recent PR that hasn't been released yet: #7518

@city41
Copy link
Author

city41 commented Jul 22, 2019

I tried rolling back that change and still saw the issue, but I'm not 100% certain on that.

Just now I copied alpha.36's client-api/dist into alpha.37's and the problem went away, so does feel like the problem is somewhere in there (client-api is primarily what changed from 36->37)

@city41
Copy link
Author

city41 commented Jul 22, 2019

I take that back. It does feel like that change might be the culprit @shilman . Here is what I did

  1. install all storybook packages at alpha.37
  2. start my storybook, confirm the bug happens
  3. cp story_store.js from @storybook/client-api@5.2.0-alpha.36/dist into 37
  4. restart storybook, bug no longer repros

@shilman
Copy link
Member

shilman commented Jul 22, 2019

@city41 I'm releasing 5.2.0-beta.6 right now with the changes I linked to above. I suspect what's going on is that you are calling load multiple times (per the documentation!) and the different calls are interfering with one another. In the new version there are facilities for achieving everything in a single call, and this is now the recommended way to use load. The new release contains updated docs & examples to reflect this. Would appreciate if you could give it a try and let me know if it solves your problem (or causes more problems! it's a pretty big change, but an important one!). I'll post here when the release is ready.

@city41
Copy link
Author

city41 commented Jul 22, 2019

Do you mean load from @storybook/react?

Here is my config.js

import { load } from '@storybook/react';

load(require.context('../packages', true, /.stories.mdx$/), module);

As far as I know, I am only calling load once.

I will try beta.6 once it's out. Thanks for looking into this!

@shilman
Copy link
Member

shilman commented Jul 22, 2019

Hmm, ok there goes that theory then! 🤷‍♂

alpha.37 fixed a bug relating to deleting stories, so for sure these codepaths are relevant to what's going on.

beta.6 also changes that code significantly. So it might fix the problem or might even make it worse! This HMR stuff is tricky and pretty hard to test.

Thanks so much for hunting down the breaking change and also for being so patient on this. Promise this is going to be amazing once the kinks are worked out 😸

@shilman
Copy link
Member

shilman commented Jul 22, 2019

@city41
Copy link
Author

city41 commented Jul 22, 2019

The bug still repros for me with beta.6

@city41
Copy link
Author

city41 commented Jul 22, 2019

Although interestingly, with beta6 it doesn't always repro. Sometimes I make a change and it succeeds to reload the stories. In alpha 37 on up, as far as I know, the bug always repros after a change.

@shilman
Copy link
Member

shilman commented Jul 22, 2019

@city41 Rats. I'll dig in on this and see if I can get a repro today.

@city41
Copy link
Author

city41 commented Jul 22, 2019

I will debug a bit more tomorrow as well. I'll also see if I can get a repro, I suspect at the very worst I could replace all my real code with dummy code but keep the story structure, and stick that on github somewhere 🤞

@shilman
Copy link
Member

shilman commented Jul 23, 2019

Here are some thoughts:

  1. Verify that there are no errors in the browser dev console. Sometimes an error midway through load can cause stories not to load. I doubt this is the problem since you didn't have a problem in older versions of Storybook, but it never hurts to check.
  2. If you set logging level to verbose in the browser console, you can see multiple calls to setStories that might give more clues as to what's going on.

Storybook

I tried to repro in the monorepo but haven't had any luck yet. If you can create a repro I'm sure I can get to the bottom of this.

@city41
Copy link
Author

city41 commented Jul 23, 2019

I'll give your debug tips a try this afternoon. In the mean time, I did create a pretty small repro:

https://github.com/city41/storybookRepro

I took my actual project and stripped out most everything. There's a lot of cruft in the package.jsons for example. But for the most part just a few stories and a few dummy components feeding them.

With this repo I can reproduce the bug.

@shilman
Copy link
Member

shilman commented Jul 23, 2019

@city41 awesome. I'll take a look today. Thanks so much for doing this!

@shilman
Copy link
Member

shilman commented Jul 24, 2019

OK figured out the problem based on your repo and am working on a fix.

The issue is that you have three files that contain the following snippet <Meta title="Hawkins/React Primitives/buttons" />. Each file should have a unique "title" (usually corresponding to the component) you're documenting.

Due to a bug we "accidentally" supported duplicating titles in early alphas (at the cost of other things, like not being able to delete stories from a kind).

I think the fix is:

  1. Document this constraint
  2. Throw an error when the user tries load the same kind twice

What do you think?

@city41
Copy link
Author

city41 commented Jul 24, 2019

Ah, I see. Thanks for tracking this down. I am pretty new to storybook and didn't realize that is a limitation.

The reason I did that as it removes one layer of nesting from the stories. I can have a tree like this

image

Otherwise it needs to be something like this

image

I understand that most of the time, a component will have more than one story, so in the general case it does make sense.

Thanks for all your help! Sorry it was just user error at the end.

@shilman
Copy link
Member

shilman commented Jul 24, 2019

No worries! It's possible to do what you were trying to do with the "classic" storiesOf API and the behavior you found is undocumented, so I'm really glad we could figure it out together. Thanks for being a guinea pig! 😉

Here's how I would hack it in MDX if I wanted your flatter structure FWIW:

<Meta title="Atoms/buttons" />

<Story name="primary"><PrimaryButton /></Story>

<Story name="secondary"><SecondaryButton /></Story>

...

@shilman shilman self-assigned this Jul 24, 2019
@city41
Copy link
Author

city41 commented Jul 24, 2019

Maybe I'll go back to the old API. The problem with putting more than one story in an MDX file is it muddies the doc tab. My goal is for each button to have its own doc tab.

Since the old API still supports MDX docs, maybe that is a better fit for me?

@shilman
Copy link
Member

shilman commented Jul 25, 2019

@city41 The storiesOf API does not support MDX (per se, you could hack it but I'd strongly recommend against this).

The right way to do this unless you really want to fight against Storybook is that each component gets its own MDX file.

We have UI updates planned that will partially address your nesting concerns, which are partially described in #6646 and #6644

Specifically, when you're running in dev mode, the extra nesting will be there, but we'll save you one click since clicking on the component will navigate to the first story.

When you static export your stories using the build-storybook --docs flag we won't show the stories within a component, so you should see what you want on a documentation site.

Both of these changes are still tentative, but will be fixed before 5.2 ships. In the future we may also allow addons to mess with the nav hierarchy, so that's a backdoor to do even more customization.

Hope that helps.

@shilman
Copy link
Member

shilman commented Jul 25, 2019

ZOMG!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-beta.8 containing PR #7542 that references this issue. Upgrade today to try it out!

You can find this prerelease on the @next NPM tag.

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

@shilman shilman closed this as completed Jul 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants