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

feat: add story indexer to support storyStoreV7 #38

Merged
merged 5 commits into from
Mar 18, 2023
Merged

feat: add story indexer to support storyStoreV7 #38

merged 5 commits into from
Mar 18, 2023

Conversation

tobiasdiez
Copy link
Owner

Fixes #30.

@tobiasdiez
Copy link
Owner Author

Todo: fix story id generation. It currently fails with

✖ Failed to extract stories from your Storybook
This is usually a problem with your published Storybook, not with Chromatic.

Build and open your Storybook locally and check the browser console for errors.
Visit your published Storybook at https://640982c419c9697b75bfd8fb-mlppulfkou.chromatic.com/
The following error was encountered while running your Storybook:

Evaluation failed: Error: Didn't find '/home/runner/work/storybook-vue-addon/storybook-vue-addon/examples/vite/src/components/Button.stories.vue--Primary' in CSF file, this is unexpected
    at StoryStore.storyFromCSFFile (https://640982c419c9697b75bfd8fb-mlppulfkou.capture.chromatic.com/sb-preview/runtime.mjs:40:9843)
    at https://640982c419c9697b75bfd8fb-mlppulfkou.capture.chromatic.com/sb-preview/runtime.mjs:40:11585
    → Failed to publish build
    at Array.reduce (<anonymous>)
    at StoryStore.extract (https://640982c419c9697b75bfd8fb-mlppulfkou.capture.chromatic.com/sb-preview/runtime.mjs:40:11464)
    at PreviewWeb.extract (https://640982c419c9697b75bfd8fb-mlppulfkou.capture.chromatic.com/sb-preview/runtime.mjs:93:199)
Error: non-zero exit code

@JReinhold
Copy link

As for the ID, I'm not sure / is allowed in story IDs. I don't think this is Chromatic crashing, but rather the built output, so if you build your Storybook locally and serve it with npx http-serve storybook-static then I would expect it to fail there too.

I think the toId function from @storybook/csf can help you here though, as it mainly sanitizes the string to be CSF compliant. See how addon-svelte-csf uses it here:
https://github.com/storybookjs/addon-svelte-csf/blob/next/src/parser/extract-stories.ts#L128

As with addon-svelte-csf you might also need to do some collision detection. When you're sanitizing IDs and similar stuff there's a risk that you'll generate the same ID for multiple stories.

And finally I'm not sure it's a good idea to use the full file path to generate the IDs for multiple reasons:

  1. It creates very long IDs, which are noticeable in the URL
  2. It creates non-deterministic IDs. The same Storybook on your machine would get different IDs than on my machine, so we eg. wouldn't be able to send links to stories to each other.
  3. In theory it leaks info about the host machine to everyone, eg. I can now see that the Storybook was built in /home/runner/work/storybook-vue-addon/. But meh? 🤷

You might get away with just using the filename instead (higher risk of collisions), or a path relative to the project root (I'm unsure how you would generate that though).
addon-svelte-csf generates them directly from the story names, which is not bulletproof either though.

I believe Storybook core combines the meta title with the story name for an ID, which might be the safest option.

@codecov
Copy link

codecov bot commented Mar 11, 2023

Codecov Report

Merging #38 (5482b8b) into main (30bbbe0) will decrease coverage by 2.10%.
The diff coverage is 94.01%.

@@            Coverage Diff             @@
##             main      #38      +/-   ##
==========================================
- Coverage   98.52%   96.42%   -2.10%     
==========================================
  Files           1        3       +2     
  Lines         203      280      +77     
  Branches       36       45       +9     
==========================================
+ Hits          200      270      +70     
- Misses          3       10       +7     
Impacted Files Coverage Δ
src/core/indexer.ts 75.00% <75.00%> (ø)
src/core/parser.ts 97.50% <97.50%> (ø)
src/core/transform.ts 100.00% <100.00%> (+1.47%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@tobiasdiez
Copy link
Owner Author

Thanks for the input. I've now went with the meta tile + story name combination, and this seems to work well. Only problem is: how do I get the automatically generated meta title if no explicit one is specified? If I understand the code correctly, then the svelte indexer falls back to the story id/name in this case, but this seems prone to run into problems (imagine having to story files exporting a story with the name and no meta title).

It feels like this area would generally profit from a bit of restructuring and putting more responsibility onto the story index generator in the upstream storybook repo. I would say the language-specific story indexer should only care about extracting the id as specified (in whatever form) by the user, and maybe adding sensible fallsbacks (like the name). Then combining these file specific ids to globally unique ones, and adding prefixes like the story meta title should happen in a consistent way an abstraction level higher. This would also make it possible to prevent id conflicts across different story file formats.

@JReinhold
Copy link

Thanks for the input. I've now went with the meta tile + story name combination, and this seems to work well. Only problem is: how do I get the automatically generated meta title if no explicit one is specified?

Sounds like you're referring to the built-in autotitle feature. It's a bit hairy to reproduce, and you'd need the stories globs to do that. Essentially it builds a project-relative path based on those globs, and then sanitizes that path. My suggestion would be to approximate it by autogenerate the title from the file name, and if the file name is something like index.stories.vue, get it from the directory name instead (native autotitle does that).

It feels like this area would generally profit from a bit of restructuring and putting more responsibility onto the story index generator in the upstream storybook repo. I would say the language-specific story indexer should only care about extracting the id as specified (in whatever form) by the user, and maybe adding sensible fallsbacks (like the name). Then combining these file specific ids to globally unique ones, and adding prefixes like the story meta title should happen in a consistent way an abstraction level higher. This would also make it possible to prevent id conflicts across different story file formats.

I think you're right in most of this. This external indexers support is very much conceptually a v0.1 and could use a lot of API changes like these for an eventual v1 at some point. This is good feedback.

@tobiasdiez
Copy link
Owner Author

Since replicating the auto-name feature seems a bit overkill at the moment, I'll merge this now as it is and wait for further improvements of the upstream api!

Thanks for your help!

@tobiasdiez tobiasdiez merged commit 421fe9a into main Mar 18, 2023
@tobiasdiez tobiasdiez deleted the indexer branch March 18, 2023 10: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.

Add vue story indexer to support storyStoryv7
2 participants