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

Support features.storyStoreV7 #126

Merged
merged 1 commit into from
Nov 18, 2021

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Oct 26, 2021

Closes #125

@joshwooding joshwooding marked this pull request as draft October 26, 2021 00:45
@joshwooding joshwooding force-pushed the support-story-store-v7 branch 3 times, most recently from ef0c5f5 to e46de50 Compare October 26, 2021 10:26
@joshwooding
Copy link
Member Author

@tmeasday Any thoughts on the current list of issues? I'm not that familiar with how storybook works but from what I can tell the useEffect in preview.tsx sets the storyId to undefined here for some reason on initial load when there wasn't a defined path:

https://github.com/storybookjs/storybook/blob/842d4793ed5becc6bfd054eb8cf4bb36d1b4892c/lib/ui/src/components/preview/preview.tsx#L164

@tmeasday
Copy link
Member

Hey @joshwooding -- if you let me know how to run it I can take a look? It isn't entirely clear what the issues mean.

Have you looked at what events are being emitted from the preview side? I'm not sure but I think that code would only emit any empty story id if the preview somehow told it to.

Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments scanning through

@joshwooding
Copy link
Member Author

joshwooding commented Oct 26, 2021

Hey @joshwooding -- if you let me know how to run it I can take a look? It isn't entirely clear what the issues mean.

Have you looked at what events are being emitted from the preview side? I'm not sure but I think that code would only emit any empty story id if the preview somehow told it to.

@tmeasday
You should be able to test it by:

  1. Pulling the branch down
  2. running yarn install in the root folder
  3. cd to './packages/example-react'
  4. run yarn storybook

@joshwooding joshwooding force-pushed the support-story-store-v7 branch 2 times, most recently from a22dfaf to fd06954 Compare October 26, 2021 14:49
@joshwooding
Copy link
Member Author

After some investigation FEATURES in the stories.ts file isn't being updated.

@joshwooding
Copy link
Member Author

@tmeasday I'm seeing some weird stuff. Not sure if it's due to the yarn setup or something else but when installing node_modules from scratch I get a prebuilt manager which does not get changed and is stuck with the features config set to {"postcss":true}.

@IanVS
Copy link
Member

IanVS commented Nov 1, 2021

Hi @joshwooding, thanks for your work on this. Is there more you're planning to do, or is it ready to review? If so, is there anything we should be aware of as we do?

@joshwooding
Copy link
Member Author

@IanVS I think there are some few kinks to iron out. Currently the current issues are:

  • Storybook uses the prebuilt manager which breaks everything. You need to delete node_modules > @storybook/manager-webpack4 > prebuild to get everything working or use --no-manager-cache (This might be an issue with the monorepo)
  • You need to refresh the page when initial loading because a 408 timeout is returned when getting @react-refresh

@joshwooding joshwooding force-pushed the support-story-store-v7 branch from 9a28bd6 to 6ecb5bf Compare November 1, 2021 21:56
@joshwooding
Copy link
Member Author

joshwooding commented Nov 3, 2021

@IanVS I'm stumped on the 408 timeout issue. It seems like the modules referenced in the iframe aren't loaded on the first render so we get a 408 error in the console and the stories don't load. Refreshing the page fixes this. Any ideas?

@eirslett Not sure if you have any ideas?

@eirslett
Copy link
Collaborator

eirslett commented Nov 3, 2021

@eirslett Not sure if you have any ideas?

We usually add any third-party dependencies from package.json to the optimizeDeps config, so they are optimized immediately on startup.

@joshwooding
Copy link
Member Author

@eirslett The 408 requests include third-party dependencies sadly :(

image

@joshwooding joshwooding force-pushed the support-story-store-v7 branch from b83c5ee to 71df6f8 Compare November 9, 2021 19:34
@joshwooding
Copy link
Member Author

The only issue I can see now is that the static storybook sets an empty path?

@joshwooding
Copy link
Member Author

The only issue I can see now is that the static storybook sets an empty path?

This is an issue with storybook-builder-vite so as far as I can see this is ready to review @IanVS 🎉

@IanVS
Copy link
Member

IanVS commented Nov 10, 2021

Thanks for sticking with this one, @joshwooding! I'll give it some 👀 as soon as I can.

In the meantime, it would be great if anyone else would be willing to try it out and play around with it, especially those using svelte or vue or other frameworks than React, which is what I'll be testing out.

@joshwooding
Copy link
Member Author

joshwooding commented Nov 10, 2021

Hmm, I can't get svelte to work in the repo. Two instances of ClientApi seem to exist for some reason.

@joshwooding
Copy link
Member Author

joshwooding commented Nov 10, 2021

.svelte story files no longer work due to https://github.com/storybookjs/storybook/blob/next/lib/core-server/src/utils/StoryIndexGenerator.ts#L68 cc @tmeasday

I assume this is intentional

storybookjs/storybook#16673 an issue has been raised to track this.

@tmeasday
Copy link
Member

Hmm, yes I guess we will need to think about that @shilman

@joshwooding joshwooding force-pushed the support-story-store-v7 branch from 71df6f8 to d146303 Compare November 15, 2021 22:45
@joshwooding joshwooding force-pushed the support-story-store-v7 branch from d146303 to 2ac8ece Compare November 15, 2021 22:50
@joshwooding
Copy link
Member Author

joshwooding commented Nov 15, 2021

.svelte story files no longer work due to https://github.com/storybookjs/storybook/blob/next/lib/core-server/src/utils/StoryIndexGenerator.ts#L68 cc @tmeasday

I assume this is intentional

storybookjs/storybook#16673 an issue has been raised to track this.

I think this and the svelte duplicate ClientAPIs are the only issues left :)

I think the duplicate ClientAPIs are due to @storybook/addon-svelte-csf including an different version

@joshwooding joshwooding force-pushed the support-story-store-v7 branch 3 times, most recently from 74aa943 to 31746ed Compare November 16, 2021 02:04
@joshwooding joshwooding force-pushed the support-story-store-v7 branch 3 times, most recently from c2efc06 to 426a654 Compare November 16, 2021 23:20
@IanVS
Copy link
Member

IanVS commented Nov 17, 2021

I pulled down the branch and am experimenting with it now. I'm seeing a few things so far, still investigating:


╭──────────────────────────────────────────────────────╮
│                                                      │
│   Storybook 6.4.0-rc.2 for React started             │
│   6.56 s for manager and 5.38 s for preview          │
│                                                      │
│    Local:            http://localhost:62893/         │
│    On your network:  http://192.168.68.116:62893/    │
│                                                      │
╰──────────────────────────────────────────────────────╯
8:25:39 PM [vite] new dependencies found: @storybook/channel-websocket, updating...
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/@storybook_preview-web.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/@storybook_client-api.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/global.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/@storybook_addons.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/@storybook_channel-postmessage.js" points to missing source files
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-JST3G223.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-XEMAM7KF.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-MRP3MKNT.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-TYR6E6LN.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-NRERFRFO.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-OAV7HHZ3.js?v=732bfa54.
8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-LRSF2QUR.js?v=732bfa54.
.
.
.
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/unfetch.js" points to missing source files
8:25:41 PM [vite] ✨ dependencies updated, reloading page...
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/@storybook_client-logger.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/telejson.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/chunk-4S6UEONU.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/util-deprecate.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/ts-dedent.js" points to missing source files
Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/chunk-AKDXD5X2.js" points to missing source files

So, it seems like we should add @storybook/channel-websocket to the optimizeDeps list, and I've seen others report the source map issues, so now that I have a reproduction I'll see if I can figure out what's going on.

After I added @storybook/channel-websocket to prebundling and restarted storybook, the messages like 8:25:39 PM [vite] Failed to load source map for /node_modules/.vite/chunk-LRSF2QUR.js?v=732bfa54. are no longer shown, but I do still see a lot like Sourcemap for "/Users/ianvs/code/storybook/storybook-builder-vite/packages/example-react/node_modules/.vite/lodash_isFunction.js" points to missing source files

OK, found a solution for those warnings: vitejs/vite#5438 (comment). I'll throw up a PR.

@IanVS
Copy link
Member

IanVS commented Nov 17, 2021

It's so cool to see the new components get fetched only when you view their stories. This should help folks with large storybooks a ton!

What do you think about merging this and in our release notes mentioning that storyStoreV7 is not supported for svelte yet. It sounds like storybook changes will be needed for it to work correctly, so there's probably no need to hold up this PR. This whole project is experimental, so I'm 👍 for releasing and getting bug reports if folks hit trouble.

Copy link
Member

@IanVS IanVS left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're willing to add @storybook/channel-websocket to the list of optimized dependencies and uncomment out the accessibility plugin in the examples, I'll give this my 👍. Great work here, thanks again for sticking with it.

@IanVS IanVS requested a review from eirslett November 17, 2021 03:03
@tmeasday
Copy link
Member

It's so cool to see the new components get fetched only when you view their stories

In Vite I would assume they are only getting compiled when requested too, which I assume should help larger storybooks a lot!

@IanVS
Copy link
Member

IanVS commented Nov 17, 2021

Compiling is really fast in vite, but one of the bottlenecks has been sending all the (sometimes hundreds of) files to the browser at startup. So for both reasons, this is a big step in the right direction!

@joshwooding joshwooding force-pushed the support-story-store-v7 branch from 426a654 to 01dbbd5 Compare November 17, 2021 12:10
@joshwooding
Copy link
Member Author

It's so cool to see the new components get fetched only when you view their stories. This should help folks with large storybooks a ton!

What do you think about merging this and in our release notes mentioning that storyStoreV7 is not supported for svelte yet. It sounds like storybook changes will be needed for it to work correctly, so there's probably no need to hold up this PR. This whole project is experimental, so I'm 👍 for releasing and getting bug reports if folks hit trouble.

Sounds great. I've added comments and updated optimizeDeps :)

@IanVS
Copy link
Member

IanVS commented Nov 17, 2021

Oh shoot, I've caused a merge conflict in the lockfile. Would you mind rebasing one more time?

@joshwooding joshwooding force-pushed the support-story-store-v7 branch from 01dbbd5 to c648994 Compare November 17, 2021 18:24
@joshwooding
Copy link
Member Author

@IanVS Done :)

@IanVS IanVS merged commit 496312e into storybookjs:main Nov 18, 2021
@joshwooding joshwooding deleted the support-story-store-v7 branch November 19, 2021 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Supporting storyStoreV7
4 participants