-
-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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: Support new indexer API #23626
Conversation
@@ -1152,6 +1152,40 @@ describe('StoryIndexGenerator with deprecated indexer API', () => { | |||
|
|||
expect(logger.warn).not.toHaveBeenCalled(); | |||
}); | |||
|
|||
it('DOES NOT throw when the same CSF file is indexed by both a deprecated and current indexer', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could have both the deprecated storyIndexers
and the new indexers
trying to index the same file, albeit it's a bit unlikely. In this scenario I've currently chosen that the new indexers takes precedence and the deprecated storyIndexers would be ignored for such a file (thus only a single index entry is generated), but I'm open to other suggestions, such as throwing an error instead.
const getStorySortParameterMock = getStorySortParameter as jest.Mock< | ||
ReturnType<typeof getStorySortParameter> | ||
>; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these two indexers below are very likely how they will be implemented for real in a follow up PR
const csf = loadCsf(code, { ...opts, fileName }).parse(); | ||
|
||
// eslint-disable-next-line no-underscore-dangle | ||
return Object.entries(csf._stories).map(([key, story]) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's unfortunate that we have to rely in the "internal" _stories
field here, but that's the only way to get stories' keys.
I could also change CsfFile.stories
to return the object instead of only the values, but I don't know if that would be considered a breaking change? Depends on if we consider CsfFile
a public API or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the key
here in this case if we already have a story.id
? Or is that not guaranteed to exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
is required in the new IndexInput
. And key
and id
is not the same, here's an example:
{
key: 'LoggedIn',
id: 'example-header--logged-in',
name: 'Logged In',
title: 'Example/Header',
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@JReinhold but in the example above, does key
do anything? Would it not just get thrown away? IIUC key
is just used in preference to name
when calculating id
when id
itself isn't passed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, key
is just used to calculate name
, name
and title
are used to calculate id
.
So in this case the key
is just to satisfy the typings that require a key
.
We could make the types more clever by requiring key
or name
, but I think we need a longer discussion about key
/name
/id
.
|
||
if (createDocEntry) { | ||
const name = this.options.docs.defaultName; | ||
// TODO: how to get "component title" or "component tags" when we only have direct stories here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as we don't have a concept of component
but only story entries here now, I don't see a way to get tags and title from the meta. I just opted to get it from the first entry.
If anyone can come up with a smarter way or a reason why this is bad, let me know.
const csf = loadCsf(code, { ...opts, fileName }).parse(); | ||
|
||
// eslint-disable-next-line no-underscore-dangle | ||
return Object.entries(csf._stories).map(([key, story]) => ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need the key
here in this case if we already have a story.id
? Or is that not guaranteed to exist?
ReturnType<typeof getStorySortParameter> | ||
>; | ||
|
||
const storiesMdxIndexer: Indexer = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this handle "docs only" .stories.mdx
files? I don't see tests for those so I am guessing maybe not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does not. I plan to add that in the follow up PR for implementing the indexers, since that logic now lives there instead.
storyStoreV7: true, | ||
docs: { defaultName: 'docs', autodocs: false }, | ||
}; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
none of the tests here are new, they've just been copied back from the deprecated test suite. The only thing new here is the indexers above
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nitpick, rest LGTM
Works on #23457
What I did
This PR adds support for the new story indexer API in
StoryIndexGenerator
. It doesn't actually convert all the existing indexers yet to the new API (except for in tests), that is a follow up PR.How to test
Create a sandbox (or use the canary release listed below) and replace the current deprecated indexers with new ones, by adding this to
main.js
:Checklist
MIGRATION.MD
Maintainers
ci:normal
,ci:merged
orci:daily
GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.ts
["cleanup", "BREAKING CHANGE", "feature request", "bug", "build", "documentation", "maintenance", "dependencies", "other"]
🦋 Canary release
This pull request has been released as version
0.0.0-pr-23626-sha-0d5a537f
. Install it by pinning all your Storybook dependencies to that version.More information
0.0.0-pr-23626-sha-0d5a537f
support-new-indexer
0d5a537f
1690454835
)To request a new release of this pull request, mention the
@storybookjs/core
team.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=23626