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

Meta/Story of not available in .stories.mdx files #20496

Closed
dev-nicolaos opened this issue Jan 4, 2023 · 13 comments · Fixed by #21615
Closed

Meta/Story of not available in .stories.mdx files #20496

dev-nicolaos opened this issue Jan 4, 2023 · 13 comments · Fixed by #21615
Assignees

Comments

@dev-nicolaos
Copy link

dev-nicolaos commented Jan 4, 2023

Describe the bug

Its possible that this is another case of #19964, but none of the examples listed there matched mine so I decided to create a new issue. When trying to run storybook start with the repo below, I get the error:

...\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:372906
    throw new Error("Expected a Story name, id, or story attribute");
          ^


Error: Expected a Story name, id, or story attribute
    at genStoryExport (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:372906:11)
    at genCanvasExports (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:373005:27)
    at <anonymous> (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:373149:23)
    at Array.forEach (<anonymous>)
    at extractExports (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:373143:18)
    at <anonymous> (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:373200:19)
    at wrapped (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:353059:16)
    at next (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:353033:22)
    at done (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:353080:7)
    at then (c:\Users\nicsk\Repositories\sb-bug-repro\node_modules\@storybook\mdx2-csf\dist\index.js:353084:5)

This example follows the recommendation in the Migration doc and the beta's docs to:

  • import from @storybook/blocks
  • use the of prop to reference stories
  • move the page's metadata from the MDX to the JS

If I undo the latter two steps and instead:

  • list the metadata as props directly on the <Meta> doc block
  • use the <Story> component's story prop to reference a story

then the build compiles successfully and I can see the stories rendering in the browser, though the docs page will still be broken.

To Reproduce

git clone https://github.com/dev-nicolaos/sb-bug-repro.git

npm start

System

Environment Info:

  System:
    OS: Windows 10 10.0.22621
    CPU: (8) x64 Intel(R) Core(TM) i7-8550U CPU @ 1.80GHz
  Binaries:
    Node: 18.12.1 - ~\AppData\Local\nvs\default\node.EXE
    npm: 8.19.2 - ~\AppData\Local\nvs\default\npm.CMD
  Browsers:
    Edge: Spartan (44.22621.963.0), Chromium (108.0.1462.54)
  npmPackages:
    @storybook/addon-essentials: ^7.0.0-beta.19 => 7.0.0-beta.19 
    @storybook/addons: ^7.0.0-beta.19 => 7.0.0-beta.19 
    @storybook/react-vite: ^7.0.0-beta.19 => 7.0.0-beta.19

Additional context

The environment info spat out by npx sb@next info is not quite correct. I'm running Windows 11 Home 22H2 and have Firefox (108.0.1) as my default browser.

@fionnachan
Copy link

facing the same error on Macbook Pro MacOS Monterey 12.5

WARN 🚨 Extraction error on src/components/atoms/button/button.stories.mdx: Error: Expected a Story name, id, or story attribute
/Users/.../node_modules/@storybook/mdx2-csf/dist/index.js:372906
    throw new Error("Expected a Story name, id, or story attribute");
          ^

Error: Expected a Story name, id, or story attribute
    at genStoryExport (/Users/.../node_modules/@storybook/mdx2-csf/dist/index.js:372906:11)

@shilman
Copy link
Member

shilman commented Feb 4, 2023

Thanks so much for your reproduction @dev-nicolaos !! It uncovered three issues.

All of these were "user errors" but which are completely impenetrable runtime errors and can be fixed with better error handling and documentation.

I'm reclassifying this as a documentation issue and hopefully @JReinhold @tmeasday @yannbf can provide some help with this. I think this is confusing and will snag a bunch of users in the upgrade process.

Meta/Story of not available in .stories.mdx files

To use the <Story of={...} /> syntax, the file must be named Button.mdx instead of Button.stories.mdx. This is a confusing restriction and something we should document better (or possibly fix?) before RC.

This should be clarified:

  • in the upgrade guide
  • In the documentation
  • ideally as a runtime check to improve the error message

Meta of incorrect

In your reproduction, when I rename Button.stories.mdx to Button.mdx, I run into #19897.

The MDX file incorrectly references <Meta of={Button} /> instead of <Meta of={stories} />.

Since numerous people have run into this issue including yourself and at least 5 people on #19897, we should clarify this:

  • in the docs
  • as a runtime check to provide a useful error message.

Specifically, CSF files should contain a default export that contains either a component or a title and if we don't see this we can warn the user about this issue.

Story of not available for stories that are not declared in main.js

In the example, you only include **.stories.mdx in main.js. However, you refer to Button.stories.jsx which is not declared in main.js.

This means that when you reference the stories.Basic, it cannot find it in the story index. This is a corner case, but difficult to understand/debug with the current error message.

This should be clarified:

  • in the docs
  • ideally as a runtime check to provide a useful error message

@shilman shilman added documentation and removed bug labels Feb 4, 2023
@shilman shilman moved this to Required for RC in Core Team Projects Feb 4, 2023
@shilman shilman changed the title [Bug]: MDX Docs are broken in SB 7 Beta Clarify that Meta/Story of syntax is only available in `.mdx files Feb 4, 2023
@shilman shilman changed the title Clarify that Meta/Story of syntax is only available in `.mdx files Clarify that Meta/Story of construct is only available in .mdx files Feb 4, 2023
@shilman shilman changed the title Clarify that Meta/Story of construct is only available in .mdx files Meta/Story of construct is only available in .mdx files Feb 4, 2023
@shilman shilman changed the title Meta/Story of construct is only available in .mdx files Meta/Story of not available in .stories.mdx files Feb 4, 2023
@shilman shilman added the bug label Feb 4, 2023
@shilman shilman moved this from Required for RC to Required for QA in Core Team Projects Feb 4, 2023
@shilman shilman changed the title Meta/Story of not available in .stories.mdx files Meta/Story of not available in .stories.mdx files Feb 4, 2023
@dev-nicolaos
Copy link
Author

@shilman thanks for your detailed response. I have a couple of thoughts about the "user errors" that you mentioned. These are from the perspective of someone who is just a user, so I understand there may be implementation details I'm unaware of that influenced these decisions.

To use the <Story of={...} /> syntax, the file must be named Button.mdx instead of Button.stories.mdx. This is a confusing restriction and something we should document better (or possibly fix?) before RC.

I agree this is a really confusing restriction, especially since the pattern for the story files is explicitly declared by the user in main.js. Documentation would help, but really seems like this should be fixed.

The MDX file incorrectly references <Meta of={Button} /> instead of <Meta of={stories} />.

Makes sense. I feel like I tried that at one point and maybe something else went wrong and I reverted to the v6 (<Meta of={Button} />) syntax while debugging. Documentation feels like the right solve here.

In the example, you only include **.stories.mdx in main.js. However, you refer to Button.stories.jsx which is not declared in main.js.

This means that when you reference the stories.Basic, it cannot find it in the story index. This is a corner case, but difficult to understand/debug with the current error message.

This is really counter-intuitive. JavaScript and most if not all of the ecosystem tooling only require an entry point/pattern to be provided and the engine/tool automatically follows the import of those files to form the dependency tree. Unless this is required due to a major technical limitation of Storybook's internals, it feels like this should be changed/fixed rather than documented. Certainly feels like a step backward from v6 where this was not required.

@tmeasday
Copy link
Member

tmeasday commented Feb 6, 2023

Thanks for the feedback @dev-nicolaos!

I agree this is a really confusing restriction, especially since the pattern for the story files is explicitly declared by the user in main.js.

So I can see it's confusing why it seems that .stories.mdx have these essentially arbitrary restrictions (from a user perspective), but the behind the scenes reason is that such files a re-compiled to CSF files and consumed that way by SB. In theory if it were possible to remove the mdx2-csf loader (that matches .stories.mdx files) -- which it isn't AFAIK -- then we would treat such files like any other .mdx files. But all the restrictions of the newer syntax would apply.

The MDX file incorrectly references <Meta of={Button} /> instead of <Meta of={stories} />.

I think we could probably make this work @shilman (maybe with a warning). I'm not sure if that's a good or a bad idea :)

This is really counter-intuitive. JavaScript and most if not all of the ecosystem tooling only require an entry point/pattern to be provided and the engine/tool automatically follows the import of those files to form the dependency tree. Unless this is required due to a major technical limitation of Storybook's internals, it feels like this should be changed/fixed rather than documented. Certainly feels like a step backward from v6 where this was not required.

What would you expect the behaviour here to be @dev-nicolaos?

Would the stories referenced from an .mdx file but not matched by main.js:stories show up in the sidebar? Or would they just be in the docs page with no way to reach them directly?

@dev-nicolaos
Copy link
Author

So I can see it's confusing why it seems that .stories.mdx have these essentially arbitrary restrictions (from a user perspective), but the behind the scenes reason is that such files a re-compiled to CSF files and consumed that way by SB.

I'm realizing some of my expectations are driven by how I think about file names/extensions. In my mind file names/extensions should serve limited purposes:

  • Provide a hint/default for language specific editor features like syntax highlighting/code folding
  • Provide a hint to developers about the file's purpose
  • Serve as a default for tooling that searches for/reads/modifies files

To me, those are the reasons it makes sense to give a file that contains stories the .stories.jsx or .stories.mdx suffix. But that third point doesn't apply when we have to specify a glob for stories in main.js. If I'm declaring a glob, it seems like it shouldn't matter what the files are called, just use what matches the glob.

The SB team might see it differently and that's ok, but that perspective is driving my expectations on a couple of these issues, including...

What would you expect the behaviour here to be @dev-nicolaos?

Would the stories referenced from an .mdx file but not matched by main.js:stories show up in the sidebar? Or would they just be in the docs page with no way to reach them directly?

I would expect the behavior to match the existing (v6) behavior (or at least my understanding of it).

  • Since the .mdx file matches the stories glob in main.js, it is the source of truth (the thing that declares stories)
    • In this case the .stories.jsx file is just a module with exports (in this case stories) that the .mdx file happens to import. If the .stories.jsx file wasn't in the source of truth's dependency import graph I would expect it to be completely ignored by SB.
  • A story shows up in the sidebar and gets its own route when it is declared in the source of truth. If an ES module/CSF file is the source of truth, it does this with the export keyword. In an mdx file a story is declared by referencing/defining it using the <Story> component

@tmeasday
Copy link
Member

tmeasday commented Feb 7, 2023

I guess one point that differs in 6 v 7 is:

In an mdx file a story is declared by referencing/defining it using the component

You cannot define stories any more in .mdx files using <Story> -- you can only reference existing stories defined elsewhere (this is a breaking change that is still supported in a deprecated way in .stories.mdx files).

So I am not quite sure that logic follows in that paradigm. Maybe it would make sense instead that the story would not appear in the sidebar but still appear in the docs file in that circumstance.

Since the .mdx file matches the stories glob in main.js, it is the source of truth (the thing that declares stories)

I would probably rename the stories glob to entries or similar to indicate that it controls what appears in the sidebar (as .mdx files don't necessarily have to have any anything to do with stories) but that's probably getting a bit in the weeds.

@dev-nicolaos
Copy link
Author

@tmeasday I think that explanation clears up a lot of the confusion. I agree the stories key could probably use some bike shedding as its currently a bit misleading (although it may be better to keep deprecate the old key rather than delete it to avoid an unneccessary breaking change?).

I think the big thing to come out of this discussion for me is the need for improved documentation. I haven't read through the docs since I did the POC in late December so maybe its gotten better, but none of this was clear at the time.

A lack of good API documentation for Storybook's config and doc blocks has actually been one of our team's biggest pain points in adopting the tool. It feels like you have to hunt and peck through various guides to find technical details. I'm not sure if this is because Storybook's target audience is a bit broader than just engineers, but it feels like clear, concise, and comprehensive API docs could sit alongside the longer prose-style content.

@JReinhold
Copy link
Contributor

A lack of good API documentation for Storybook's config and doc blocks has actually been one of our team's biggest pain points in adopting the tool. It feels like you have to hunt and peck through various guides to find technical details. I'm not sure if this is because Storybook's target audience is a bit broader than just engineers, but it feels like clear, concise, and comprehensive API docs could sit alongside the longer prose-style content.

This is good feedback, and we're actually starting to address this specifically. There is a need for more reference-type documentation alongside the existing scenario-based documentation. Luckily our first iteration of this will actually focus on the the doc blocks and docs features, which should be published in the coming weeks.

@shilman shilman moved this to Required for GA in Core Team Projects Feb 17, 2023
@shilman
Copy link
Member

shilman commented Feb 21, 2023

Story of not available for stories that are not declared in main.js

I ran into this problem again in another design system today @JReinhold @tmeasday. The error was:

Error: Could not find "./ag-grid.stories" for docs file "src/stories/themes/ag-grid.mdx".

I think it should say something more like:

Error: No "./ag-grid.stories" for docs file "src/stories/themes/ag-grid.mdx". Check your main.js stories glob?

It's very confusing even when you know exactly how the new docs works.

@JReinhold
Copy link
Contributor

@jonniebigodes I need your input here on the docs changes we might want, based on #20496 (comment):

Meta/Story of prop not supported in .stories.mdx files
To use the <Story of={...} /> syntax, the file must be named Button.mdx instead of Button.stories.mdx. This is a confusing restriction and something we should document better (or possibly fix?) before RC.

AFAIK we don't really mention .stories.mdx files in 7.0 docs, so I'm unsure where this would be documented.
But an important thing to document might be that .mdx and .stories.mdx files behaves very differently. As a user (especially a new one starting out with 7.0) they might be compelled to name their MDX files with .stories.mdx for consistency, which won't work. I think we should document that somehow, somewhere?

Meta of incorrect
The MDX file incorrectly references <Meta of={Button} /> instead of <Meta of={stories} />.
Since numerous people have run into this issue including yourself and at least 5 people on #19897, we should clarify this.

Should we have an explicit callout box at https://storybook.js.org/docs/7.0/react/writing-docs/mdx#anatomy-of-mdx that mentions the <Meta of={} /> prop needs to be a CSF file/meta export, and not the component?

Story of can't reference stories that are not declared in main.js
You refer to Button.stories.jsx which is not declared in main.js.

I think we want to explicitly mention at https://storybook.js.org/docs/7.0/react/writing-docs/mdx#anatomy-of-mdx that any stories you reference from MDX files needs to be indexed, ie. they need to be part of the Storybook with the stories glob.

@jonniebigodes would you be okay with driving these docs changes?

@jonniebigodes
Copy link
Contributor

@JReinhold more than glad to do it. I was thinking that for items 1 and 3, we could extend the troubleshooting section and provide clarity on it. We could do a follow-up pull request based on #21615 to point at the documentation. Will that work out for you?

@JReinhold JReinhold moved this from Required for GA to In Progress in Core Team Projects Mar 16, 2023
@JReinhold
Copy link
Contributor

@jonniebigodes sounds like a good solution!

@github-project-automation github-project-automation bot moved this from In Progress to Done in Core Team Projects Mar 16, 2023
@JReinhold JReinhold reopened this Mar 16, 2023
@JReinhold JReinhold moved this from Done to In Progress in Core Team Projects Mar 16, 2023
@JReinhold JReinhold assigned jonniebigodes and unassigned JReinhold Mar 16, 2023
@shilman
Copy link
Member

shilman commented Mar 17, 2023

Gadzooks!! I just released https://github.com/storybookjs/storybook/releases/tag/v7.0.0-rc.4 containing PR #21615 that references this issue. Upgrade today to the @next NPM tag to try it out!

npx sb@next upgrade --prerelease

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

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

Successfully merging a pull request may close this issue.

6 participants