Storybook: Tree of stories loses order on HMR

[snowystinger]

Describe the bug
Tree of stories loses order on hmr

To Reproduce
Steps to reproduce the behavior:

1. Run storybook
2. Edit a story
3. Watch that story drop to the bottom of the tree

Expected behavior
Tree should stay in order

Screenshots

Code snippets
If applicable, add code samples to help explain your problem.

System:

• OS: MacOS
• Device: Macbook Pro 2018
• Browser: chrome, safari
• Framework: react
• Addons:
• Version: 5.1.0-alpha.22

Additional context
Add any other context about the problem here.

[shilman]

Related: #5827

[snowystinger]

More information on this
https://github.com/storybooks/storybook/blob/next/lib/client-api/src/story_store.js#L141 addStory is called. When a file is updated https://github.com/storybooks/storybook/blob/next/lib/client-api/src/story_store.js#L136 remove is called and then https://github.com/storybooks/storybook/blob/next/lib/client-api/src/story_store.js#L141 addStory again.

This is important because the rendering of the stories is done using https://github.com/storybooks/storybook/blob/next/lib/ui/src/components/sidebar/treeview/utils.js#L8 which loops over the object in the order in which keys were added.
So our render order cannot be preservable because import order can change from run to run.

The most straight forward solution would probably be to copy the entire this._data removing and re-adding all the keys for each "addStory" via a sorting function from global options. The good news is it's not a real copy, so it shouldn't take that long. The bad news is that it would happen a lot during start up... It could also affect a large portion of the tree if some core file was edited and invalidated a whole bunch of stories.

Otherwise, we could use a sort function during the tree rendering, I don't know what kind of performance hit we'd take with that. It'd be more predictable though as the sort would happen every time and we could potentially memo it.

Thoughts?

[shilman]

IMO the right solution is to support a sort function. Users want it, and then it ends any questions about what the order should be.

[stale[bot]]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[shilman]

@tmeasday does this fall under your purview?

[tmeasday]

@snowystinger sorry I did not comment on this sooner. As I mentioned here: https://github.com/storybooks/storybook/pull/6472#issuecomment-488481890 I don't think sorting in the treeview is correct, and I think probably sorting on the manager side in setStories is the way to go. (We made a decision, for better or worse, not to parse kind names in the preview so sorting there doesn't work)

Another small thing is that I noticed here that the sort order of roots is currently inconsistent across browsers due to a miswritten .sort() function.

While we fix this issue it would make sense to reimplement sortStoriesByKind (https://github.com/storybooks/storybook/issues/5827) -- I suppose this is what you are doing by default now.

I am not sure if it's going to fly with users to remove the current default (sort stories by import order), even if it does have this bug right now.

---

I wonder if we can fix this issue another way (without sorting). One hacky solution would be:

1. When removing, instead of delete hash[id] we do hash[id] = removedSymbol;
2. When publishing stories, filter out any removed symbols.

@shilman I think we should keep this problem in mind when thinking about how to handle HMR for load-ed stories.

[tmeasday]

@snowystinger feel free to reach out on discord if you want to get into the details of this together.

[shilman]

@tmeasday love the hack 💯

[tmeasday]

Actually I thought a bit more about this in discussions with @shilman and I'm thinking we could after all sort the stories in the preview before sending them over the wire.

Basically the format that goes over the wire is a large array of stories (although it is actually an object). We could in fact sort the stories in the extract() function (using the path/root separator as required).

If the stories enter the manager in the correct order then no special sorting would be required to make the treeview look right. I think this is the way to go because it makes other code that is interested in the story order able to rely on extract etc rather than having to re-implement sorting. It'll also allow us to support a custom sorting function defined in config.js.

[stale[bot]]

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

[beshanoe]

Hi there. This bug seems pretty serious, it ruins the whole nice experience of developing in storybook. Are you waiting for PR? Is there a solution you agreed on?
This is probably it: https://github.com/storybooks/storybook/pull/6472

[tmeasday]

Hi @beshanoe - yes, you are correct, that PR contains a fix, and will be merged into our main release branch sometime after 5.1 is released (very soon). I'm not quite sure when the fix will make it out.

[shilman]

Whoopee!! I just released https://github.com/storybookjs/storybook/releases/tag/v5.2.0-alpha.31 containing PR #6472 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.