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

Core, Server, Addon-docs: Replace story indexers with new API #23660

Merged
merged 17 commits into from Aug 2, 2023

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Jul 31, 2023

Works on #23457

What I did

This PR does multiple things (shame on me. Sorry.)

1. Replaces all the internal story indexes with the new API:

  • CSF indexer
  • Stories MDX indexer
  • Server indexer

2. Handle docsOnly mode in Stories MDX

The new indexer handler now automatically removes story entries that are docsOnly*, as defined by having the "docsOnly" tag. So the Stories MDX indexer now adds that tag when it sees parameters.docsOnly, which is actually set by CsfFile.parse() 🥵.

* docsOnly is a feature exclusive to Stories MDX, and happens whenever any stories.mdx files don't render any stories - so only Docs needs to appear in the sidebar. I assume this handling can be removed when we finally remove Stories MDX completely.

3. Minor improvements to the server renderer

  • The default stories glob when initing a server project now includes yaml and yml extensions along with the existing json extensions. The indexer already handled this, it just wasn't in the globs
  • The default stories for Page have now been converted to YAML to showcase/test this behavior
  • The new indexer adds support for tags, and autodocs works mostly

Two aspects of autodocs that are broken in the server renderer:

  • Any (iframed) stories beyond the first three always hangs forever in loading, consistently
  • Controls don't work, they don't trigger any changes

Are these two points enough that I should remove it again, or is this better than nothing @shilman ?

How to test

  1. See Chromatic doesn't report any changes to stories (especially changing names, ids), as the new indexers should produce the same result.
  2. See especially the unattached story at ?path=/docs/addons-docs-stories-mdx-unattached--docs. It should be a lonely unattached Docs entry with no stories, because it's docsOnly.
  3. For the server changes you have to create the sandbox manually, by checking out this repo and creating the internal/server-webpack5 sandbox.

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"]

🦋 Canary release

This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the @storybookjs/core team here.

core team members can create a canary release here or locally with gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>

@JReinhold JReinhold changed the title Core, Server, Addon-docs: Replace indexers with new API Core, Server, Addon-docs: Replace _story_ indexers with new API Jul 31, 2023
@JReinhold JReinhold changed the title Core, Server, Addon-docs: Replace _story_ indexers with new API Core, Server, Addon-docs: Replace story indexers with new API Jul 31, 2023
@JReinhold JReinhold marked this pull request as ready for review July 31, 2023 09:38
@JReinhold JReinhold self-assigned this Jul 31, 2023
@JReinhold JReinhold added core addon: docs server ci:daily Run the CI jobs that normally run in the daily job. labels Jul 31, 2023
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 good but I have some questions.

code/addons/docs/src/preset.ts Outdated Show resolved Hide resolved
code/lib/core-server/src/utils/StoryIndexGenerator.test.ts Outdated Show resolved Hide resolved
code/lib/core-server/src/utils/StoryIndexGenerator.ts Outdated Show resolved Hide resolved
@JReinhold JReinhold requested a review from tmeasday August 1, 2023 07:48
Copy link
Member

@shilman shilman 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! One suggestion though

import type { Indexer, NormalizedStoriesSpecifier, StoryIndexEntry } from '@storybook/types';
import { loadCsf, getStorySortParameter } from '@storybook/csf-tools';
import type { NormalizedStoriesSpecifier, StoryIndexEntry } from '@storybook/types';
// import * as csfTools from '@storybook/csf-tools';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// import * as csfTools from '@storybook/csf-tools';

const csf = (await readCsf(fileName, options)).parse();

// eslint-disable-next-line no-underscore-dangle
return Object.entries(csf._stories).map(([exportName, story]) => {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add this to a getter on CsfFile? The code seems to be duplicated all over the place and that would make this PR a lot more DRY.

Copy link
Member

Choose a reason for hiding this comment

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

CsfFile.indexEntries?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I've been thinking something similar, I'll experiment with a getter for indexInputs and stories with exportName so you don't need the funky use of _stories.

@JReinhold JReinhold merged commit 2952a90 into indexer-api Aug 2, 2023
94 of 95 checks passed
@JReinhold JReinhold deleted the replace-indexers branch August 2, 2023 22:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: docs ci:daily Run the CI jobs that normally run in the daily job. core server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants