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

Autodocs: Fix docs pages ignoring meta.id when calculating their ID #23520

Merged
merged 10 commits into from Aug 1, 2023

Conversation

sookmax
Copy link
Contributor

@sookmax sookmax commented Jul 19, 2023

Closes #23394

What I did

I made a little adjustment in StoryIndexGenerator so that the docs pages that are automatically generated would also prioritize Meta.id over Meta.title if Meta.id was provided.

I referenced code section below for the adjustment.

self._stories = entries.reduce((acc, [key, story]) => {
if (isExportStory(key, self._meta as StaticMeta)) {
const id = toId((self._meta?.id || self._meta?.title) as string, storyNameFromExport(key));
const parameters: Record<string, any> = { ...story.parameters, __id: id };
const { includeStories } = self._meta || {};
if (
key === '__page' &&
(entries.length === 1 || (Array.isArray(includeStories) && includeStories.length === 1))
) {
parameters.docsOnly = true;
}
acc[key] = { ...story, id, parameters };
const { tags, play } = self._storyAnnotations[key];
if (tags) {
const node = t.isIdentifier(tags)
? findVarInitialization(tags.name, this._ast.program)
: tags;
acc[key].tags = parseTags(node);
}
if (play) {
acc[key].tags = [...(acc[key].tags || []), 'play-fn'];
}
}
return acc;
}, {} as Record<string, StaticStory>);

How to test

  1. Run a sandbox for template, e.g. yarn task --task sandbox --start-from auto --template react-vite/default-ts
  2. Add id to meta object in Button.stories.ts (e.g., MyButton)
  3. Open Storybook in your browser
  4. Go to Button docs page and confirm the url is something like: http://localhost:6007/?path=/docs/mybutton--docs

Checklist

  • Make sure your changes are tested (stories and/or unit, integration, or end-to-end tests)
  • Make sure to add/update documentation regarding your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli/src/sandbox-templates.ts
  • Make sure this PR contains one of the labels below.

["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]

@sookmax
Copy link
Contributor Author

sookmax commented Jul 20, 2023

The second commit includes a fix for the case of MDX docs. It is slightly more involved than the case of autodocs since MDX docs, when being processed, don't have a direct access to csf.meta.id.

I decided to store the whole csf.meta object in StoryIndexEntry and pull the value (meta.id) from there, which inevitably resulted in generic index signature being added to StoryIndexEntry type, and the snapshots in code/lib/core-server/src/utils/StoryIndexGenerator.test.ts being updated.

I am not sure the decisions I made for the second commit are sound, so yes definitely need some reviews!

Thanks.

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.

Let's drop the meta again in the getIndex, maybe something like:

          // Drop the meta as it isn't part of the index, we just used it for record keeping
          const { meta, ...existing } = indexEntries[entry.id];

Also, can we add a test case to storyIndexGenerator.test.ts that uses a component id?

@sookmax
Copy link
Contributor Author

sookmax commented Jul 21, 2023

@tmeasday Hello, thanks for the review! I very much appreciate it.

I pushed additional commits reflecting what you suggested. I decided to filter out the meta before returning in ensureExtracted. I thought that way the consumers of ensureExtracted (e.g., getIndex()) that use its return value wouldn't see the meta. And as a result the updates to the snapshots weren't needed anymore so I reverted the snapshots.

I'm now going to work on the test cases and I'll let you know when I'm done!

Thanks.

@sookmax
Copy link
Contributor Author

sookmax commented Jul 21, 2023

@tmeasday I added two test cases (for autodocs and mdx docs) under describe('autodocs') and describe('docs specifier') respectively. I chose to put relevant mock data in a new directory called docs-id-generation under utils/__mockdata__ since I didn't want them to be grabbed by ./src/**/*.stories.(ts|js|mjs|jsx), which seemed to break a few other test cases if they were 😅.

Let me know if you prefer other directory/file structures for mock data.

Thanks!

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.

Great job @sookmax!

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Looks good! Only a minor request regarding the types.

code/lib/types/src/modules/storyIndex.ts Outdated Show resolved Hide resolved
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.

Looks great!

@ndelangen
Copy link
Member

@sookmax Looks like some merge-conflicts happened, do you think you'd be able to resolve those?

@sookmax
Copy link
Contributor Author

sookmax commented Jul 26, 2023

@ndelangen Yes sure! It doesn't appear to be too complex so let me look into it, and I'll let you know when resolved!

@sookmax sookmax requested a review from yannbf as a code owner July 26, 2023 13:01
@sookmax
Copy link
Contributor Author

sookmax commented Jul 26, 2023

@ndelangen Merge is done!

@sookmax sookmax force-pushed the prioritize-id-over-title-docs branch from 63a45d2 to c334641 Compare July 27, 2023 06:46
@sookmax
Copy link
Contributor Author

sookmax commented Jul 27, 2023

@ndelangen the previous merge seemed accidentally undo 1e90354 😅. So I re-merged and force-pushed the new merge.

@ndelangen ndelangen changed the title fix: prioritize Meta.id over Meta.title for ids of docs pages Autodocs: prioritize Meta.id over Meta.title for ids of docs pages Jul 31, 2023
@ndelangen ndelangen changed the title Autodocs: prioritize Meta.id over Meta.title for ids of docs pages Autodocs: Prioritize Meta.id over Meta.title for ids of docs pages Jul 31, 2023
@ndelangen ndelangen changed the title Autodocs: Prioritize Meta.id over Meta.title for ids of docs pages Autodocs: Prioritize Meta.id over Meta.title for ids of docs pages Jul 31, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

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

Great work! 💪

I'm currently working on a big overhaul of StoryIndexGenerator, and I'll make sure to merge your changes into that.

@JReinhold JReinhold merged commit bdbd335 into storybookjs:next Aug 1, 2023
45 of 48 checks passed
@github-actions github-actions bot mentioned this pull request Aug 1, 2023
14 tasks
@sookmax
Copy link
Contributor Author

sookmax commented Aug 2, 2023

@JReinhold Ah, is that related to #23457?

Looking forward to learning the new architecture when it's done! 👍

@JReinhold JReinhold added bug and removed maintenance User-facing maintenance tasks labels Aug 2, 2023
@JReinhold JReinhold changed the title Autodocs: Prioritize Meta.id over Meta.title for ids of docs pages Autodocs: Fix docs pages ignoring meta.id when calculating their ID Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Cannot manually set story id for the Docs pages
4 participants