Indexer: Deprecate `storyIndexers` API, add foundation for `indexers` #23582

JReinhold · 2023-07-24T20:38:22Z

Works on #23457

What I did

Deprecated existing storyIndexers in favor of indexers as the new entry for the new API
Doesn't actually implement support for indexers yet, they are ignored. Which is also why this doesn't target next - we don't want to deprecate the API before the replacement is ready.
Move tests for StoryIndexGenerator to StoryIndexGenerator.deprecate.test.ts. The plan is to copy the file to StoryIndexGenerator.test.ts that will have the same tests but with the new API instead (plus a bunch more)

Follow-up PR here will implement support for the new IndexEntry in StoryIndexGenerator, which should be enough to make this releasable. Actually changing our internal indexers can be done afterwards.

How to test

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

JReinhold · 2023-07-24T20:44:58Z

code/lib/core-server/src/utils/StoryIndexGenerator.deprecated.test.ts

  storiesV2Compatibility: false,
  storyStoreV7: true,
  docs: { defaultName: 'docs', autodocs: false },
 };

-describe('StoryIndexGenerator', () => {
+describe('StoryIndexGenerator with deprecated indexer API', () => {


also notice the file rename here.

JReinhold · 2023-07-24T20:56:53Z

code/lib/csf-tools/src/CsfFile.ts

+  // TODO: is there a better way to type this correctly?
+  parse(): CsfFile & { meta: StaticMeta; stories: IndexedStory[] } {


Would like some input here @kasperpeulen @tmeasday

There's no new implementation, I just added types to our existing indexers which surfaced this type error.
The problem is, that:

Most indexers return the result CsfFile.parse() as the result of the indexer.

An indexer needs to return an IndexedCsfFile

CsfFile.parse() returns CsfFile, not IndexedCsfFile

The difference between the two is that:

meta can be undefined in CsfFile, it can't in IndexedCsfFile

stories.*.name can be undefined in CsfFile (StaticStory), it can't in IndexedCsfFile (IndexedStory)

The implementation of CsfFile.parse() ensures that both of these constraints are met, but TypeScript of course doesn't know that because 3.

My solution here makes TypeScript happy, but it doesn't feel like a type-safe way to solve this?

Is there a reason you didn't just change it to return an IndexedCSFFile (I assume that's what you meant?)

IndexedCSFFile has less properties than CsfFile, so other callsites that uses parse() won't accept that. But I can change it to CsfFile & IndexedCSFFile which should be okay I guess.

I don't really know the difference between the two types but is another option to just add the fields to CsfFile if that's what's returned by the CsfFile library?

The issue is that CsfFile already have meta and stories, but they can be undefined because they are not set during construction, only after parse() is called.

JReinhold · 2023-07-24T21:04:41Z

code/lib/types/src/modules/indexer.ts

 export type IndexEntry = StoryIndexEntry | DocsIndexEntry;
+export interface NewBaseIndexEntry {


It's annoying that we have to temporarily introduce this new type, but it's the only non-breaking way I can think of. We'll replace IndexEntry with NewIndexEntry in 8.0.

If users migrate now, they'll migrate to NewIndexEntry, only to migrate back to IndexEntry in 8.0, which must be annoying.

I think this type isn't an IndexEntry. And index entry is what ends up in index.json, so should for instance always include an id.

I think this type is like an IndexEntryInput or something?

PS -- tell me more about key. For CSF files we don't need that because it's expected that the loader will produce a CSFFile which is keyed on storyId. That is the loader responsible for mapping the pure ESM module exports to an object keyed on storyId, so we can look up the story annotations only knowing the storyId.

Are we expecting something different in this case?

I think this type isn't an IndexEntry. And index entry is what ends up in index.json, so should for instance always include an id.

I think this type is like an IndexEntryInput or something?

I see what you mean, of course! I've changed it to that, then IndexEntry also won't be deprecated.

PS -- tell me more about key. For CSF files we don't need that because it's expected that the loader will produce a CSFFile which is keyed on storyId. That is the loader responsible for mapping the pure ESM module exports to an object keyed on storyId, so we can look up the story annotations only knowing the storyId.

Are we expecting something different in this case?

This type is taken directly from the Indexer RFC, which also shows example indexers that makes use of the key property.
I'm assuming that this is added to make indexers and story definitions more flexible, in that the export doesn't need to match the story ID, but I'd like @shilman to elaborate here.

Are you saying that if we introduce this, we need to make the annotation lookup smarter, because then it can't just rely on the id, it also needs to know the key, which I guess would mean we need the key in the index as well?

I guess would mean we need the key in the index as well?

Yes, if the plan is to read the export named key in the browser when constructing the NormalizedCsfFile. However, I'm not sure if that makes sense because I'm pretty sure you are going to need to transform the raw module exports (of the indexed file) to get something that looks like the module exports of a "true" CSF file anyway (either in a loader or a runtime transformer ala this comment).

If you are already transforming the file into CSF, why wouldn't you transform the export names to mean this key business wasn't required (ie. so they match up with the story ids)? Alternatively, why wouldn't you just write the story ids in the first place to match with the export names?

Perhaps I am missing a use case though.

It's been awhile since I wrote the RFC but I think the rationale was twofold.

First, the consumer of the data needs to have the export name to be able to reference the story:

export default { title: 'Button' }; export const Primary = {};

This would generate a story with ID button--primary. But we don't know if the export key is Primary, primary, or something else. I'm not sure how this is handled today, but I felt that providing the key directly would be more straightforward.

Second, it is more convenient to provide the key and have the calling code generate the story ID automatically from the export. And if you provide the ID that can take precedence.

If it's causing problems, I'm fine with using something closer to the current interface instead. But I'd like to know how it actually works currently, since it seems like it would be a bit convoluted.

Ok, let me try again. Currently, the indexer must produce an id.

Separately, the code that loads the story computes its own id here (key is called exportName here):

storybook/code/lib/preview-api/src/modules/store/csf/normalizeStory.ts

Line 55 in 5cb57a8

const id = parameters.__id || toId(meta.id, exportName);

It's convoluted because the system currently assumes that the ID generation is identical across both codepaths, and if not then it will fail.

The minimal change would be that the Indexer can return a key, but that the code that calls the indexer computes the id from the title and key (unless an id is also provided), but that the key is not stored in the index.

That seems simple enough and uncontroversial. I'm not sure how it interacts with auto title but suspect in a good way.

This looks like a good design. To make sure I understood it correctly, let's continue the example above: The vue indexer would handle the story export const vueStoryDefault by returning id = default and key = vueStoryDefault. Then the url would use the id, but the loader the key, right?

In that context, it would be nice if the indexer doesn't need to handle id/key conflicts but instead that this would be handled in a central place. For example, the svelte indexer currently keeps track of all ids its encounters and prevents id collisions (e.g. https://github.com/storybookjs/addon-svelte-csf/blob/da2523a272716884667eb6107d6a5d95ab38d706/src/parser/extract-id.ts#L13-L37). In my opinion, this adds unnecessary complications for the indexer (every indexer would need to implement a similar feature) and doesn't work if multiple indexers are used (an indexer doesn't know about the ids that another indexer creates).

The minimal change would be that the Indexer can return a key, but that the code that calls the indexer computes the id from the title and key (unless an id is also provided), but that the key is not stored in the index.

Now that I've implemented this and thought about it some more, this still doesn't quite make sense to me @shilman.

with this solution you're still computing the export name from the id (because that is all there is in the index), so the id must directly match the export name. so if you input a key that matches the export, but write a custom id like my-pretty-id, then SB will throw because it can't find the MyPrettyId export in the CSF.

I'm not sure what the point is of the indexer being able to specify the id, since it must match the same ID generation as you pointed out.

In that context, it would be nice if the indexer doesn't need to handle id/key conflicts but instead that this would be handled in a central place. ... In my opinion, this adds unnecessary complications for the indexer (every indexer would need to implement a similar feature) and doesn't work if multiple indexers are used (an indexer doesn't know about the ids that another indexer creates).

Agree @tobiasdiez, this makes sense to me too. Stretch goal @shilman ?

tmeasday

Lgtm

JReinhold added 7 commits July 24, 2023 11:59

add indexers config, deprecate storyIndexers

380ee68

Add Indexer type

e1d732f

add types for new indexer

239d89d

add test for deprecated API

7051481

add new index type, move StoryIndexGenerator tests

3fcb9df

revert back to storyIndexers

ba63c5f

add support for storyIndexers and indexers in StoryIndexGenerator

9061c6e

JReinhold commented Jul 24, 2023

View reviewed changes

improve type docs

73a32be

JReinhold commented Jul 24, 2023

View reviewed changes

JReinhold marked this pull request as ready for review July 24, 2023 21:05

JReinhold requested review from shilman, valentinpalkovic, kasperpeulen, ndelangen, tmeasday, kylegach and jonniebigodes as code owners July 24, 2023 21:05

JReinhold assigned JReinhold and kasperpeulen Jul 24, 2023

JReinhold added feature request ci:normal labels Jul 24, 2023

JReinhold changed the title ~~Indexer: deprecate storyIndexers API, add foundation for indexers~~ Indexer: Deprecate storyIndexers API, add foundation for indexers Jul 24, 2023

JReinhold added 2 commits July 24, 2023 23:23

fix unit tests

fd9a895

separate IndexEntry from new IndexInput - improve CsfFile.parse type

41d8e12

tmeasday approved these changes Jul 26, 2023

View reviewed changes

JReinhold merged commit 898f019 into indexer-api Jul 26, 2023
49 of 50 checks passed

JReinhold deleted the new-indexer-api branch July 26, 2023 10:04

JReinhold mentioned this pull request Aug 2, 2023

Indexing: Introduce new experimental indexer API - NESTED PR! #23691

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Indexer: Deprecate `storyIndexers` API, add foundation for `indexers` #23582

Indexer: Deprecate `storyIndexers` API, add foundation for `indexers` #23582

JReinhold commented Jul 24, 2023 •

edited

JReinhold Jul 24, 2023

JReinhold Jul 24, 2023

tmeasday Jul 24, 2023

JReinhold Jul 25, 2023

tmeasday Jul 25, 2023

JReinhold Jul 25, 2023

JReinhold Jul 24, 2023

tmeasday Jul 24, 2023 •

edited

JReinhold Jul 25, 2023

tmeasday Jul 25, 2023 •

edited

shilman Jul 25, 2023 •

edited

shilman Jul 25, 2023

tmeasday Jul 25, 2023

tobiasdiez Jul 26, 2023

JReinhold Jul 26, 2023 •

edited

JReinhold Jul 26, 2023

tmeasday left a comment

		// TODO: is there a better way to type this correctly?
		parse(): CsfFile & { meta: StaticMeta; stories: IndexedStory[] } {

		export type IndexEntry = StoryIndexEntry \| DocsIndexEntry;
		export interface NewBaseIndexEntry {

Indexer: Deprecate storyIndexers API, add foundation for indexers #23582

Indexer: Deprecate storyIndexers API, add foundation for indexers #23582

Conversation

JReinhold commented Jul 24, 2023 • edited

What I did

How to test

Checklist

Maintainers

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday Jul 24, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday Jul 25, 2023 • edited

Choose a reason for hiding this comment

shilman Jul 25, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JReinhold Jul 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmeasday left a comment

Choose a reason for hiding this comment

Indexer: Deprecate `storyIndexers` API, add foundation for `indexers` #23582

Indexer: Deprecate `storyIndexers` API, add foundation for `indexers` #23582

JReinhold commented Jul 24, 2023 •

edited

tmeasday Jul 24, 2023 •

edited

tmeasday Jul 25, 2023 •

edited

shilman Jul 25, 2023 •

edited

JReinhold Jul 26, 2023 •

edited