[Bug]: experimental_indexers ignores importPath

fredriknils commented 8 months ago

Describe the bug

In the experimental indexer docs it says that an indexer can specify an importPath to a target CSF file, but doing so has no effect. Looking at the source code it looks like the importPath returned from an indexer is not used and instead the input source to the indexer will always be used as the importPath.

To Reproduce

https://github.com/storybookjs/storybook/blob/e942e71e1f5281f9197faab6d61625b0ac8cecfd/code/lib/core-server/src/utils/StoryIndexGenerator.ts#L302

System

Storybook Environment Info:

  System:
    OS: macOS 14.2.1
    CPU: (10) arm64 Apple M1 Pro
    Shell: 5.1.16 - /opt/homebrew/bin/bash
  Binaries:
    Node: 20.10.0 - ~/.nvm/versions/node/v20.10.0/bin/node
    Yarn: 4.0.2 - ~/.nvm/versions/node/v20.10.0/bin/yarn <----- active
    npm: 10.2.3 - ~/.nvm/versions/node/v20.10.0/bin/npm
  Browsers:
    Chrome: 120.0.6099.109
    Edge: 120.0.2210.121
    Safari: 17.2.1

Additional context

No response

shilman commented 8 months ago

@fredriknils indeed that looks like a bug. i'm curious what's your use case where input.path is different from the story file path?

fredriknils commented 8 months ago

@shilman I’m currently investigating if we can use the indexer api to improve our DX around creating regression stories. In our project we need to create permutations/variants of component to fully regression test out components using chromatic.

To make it easier to explain what I’m trying to do I will referee to the permutations as light and dark theme below, but I our case the permutations are in the range of 6-10 per component.

Currently we need to explicitly define a story per permutation in our regression stories file so we have something like this.

const MyComponent = () => <div>MyComponent</div>;

type Story = StoryObj<typeof meta>;

const meta: Meta<typeof MyComponent> = {
  component: MyComponent,
};

export default meta;

export const Dark: Story = {
  tags: ["variant:dark"],
};

export const Light: Story = {
  tags: ["variant:light"],
};

The "problem" with this is that if we add a new permutation, we need to go into each regression story file and add a new export. It not big thing to do but devs tend to forget to do it 😃

So, what I was trying to do when running into the importPath issue was this.

Devs create a mycomponent.regression.tsx that looks like this;

export default () => <div>MyComponent</div>;

Then I created an indexer like this

import fs from "node:fs";
import path from "node:path";

import { loadCsf } from "@storybook/csf-tools";

const regressionIndexer: Indexer = {
  test: /regression.tsx$/,
  createIndex: async (filePath, { makeTitle }) => {
    const regressionStoryAbsolutePath = filePath.replace("regression.tsx", "regression.story.tsx");
    const regressionStoryRelativePath = path.relative(process.cwd(), regressionStoryAbsolutePath);

    // Generate regression story csf file
    generateStoryFile(filePath, regressionStoryAbsolutePath);

    return loadCsf(fs.readFileSync(regressionStoryAbsolutePath, "utf-8"), {
      fileName: regressionStoryRelativePath,
      makeTitle,
    }).parse().indexInputs;
};

The indexer will auto generate the regression story file mycomponent.regression.story.tsx from the source mycomponent.regression.tsx. The generated mycomponent.regression.story.tsx looks like this;

import MyComponent from "./mycomponent.regression"

type Story = StoryObj<typeof meta>;

const meta: Meta<typeof MyComponent> = {
  component: MyComponent,
};

export default meta;

export const Dark: Story = {
  tags: ["variant:dark"],
};

export const Light: Story = {
  tags: ["variant:light"],
};

But because importPath is not currently respected this doesn’t work because storybooks will look for the stories in the wrong CSF file (might be other problems with my solution that i don't see yet 😄 ).

(What I'm trying to do with the indexer can be done by running the code-gen stuff in main.js but I was trying to use the indexer to make it more integrated into storybook flow, if possible.)

shilman commented 8 months ago

@fredriknils this is a fascinating direction. here's what i'd suggest. if you can create a PR that fixes the bug and get the tests passing, i can release a canary release of the fix that you can play with and we can evaluate what the impact of merging the fix would be -- it's possible that the current code relies on this "bug" to work.

another option is that you could consider generating those exports "in place" in the file. so you'd have a webpack or vite plugin (or better yet an unplugin) that reads the source & appends code to the file.

i also made a small proof of concept linked here, if you haven't already seen it https://github.com/storybookjs/storybook/discussions/23177

fredriknils commented 8 months ago

I had a look at fixing the “bug” and was testing it out on my local storybook. But after changing the behavior I realize that what I was hoping to achieve with the importPath will not work.

My first thinking with changing the import path was that I would avoid having to create my own builder and instead piggyback on the existing one. But after fixing the importPath I realize that the builder api specifies that the builders should read the stories array in the config not the store index entries. So, if I change the importPath the builder will not know what file I’m referring to. This can be solved by also including my generated file in the stories array but then I need to also create a secondary indexer, who’s only purpose would be to stop the generated file from being indexed, so that the builders has both the generated file and the source file, but the storybook index only has one version indexed. The whole solution feels like a hack and more complicated than just creating a builder, so I’m abandoning that approach in favor of you second suggestion of adding a builder like you did in you POC.

If anyone comes across this thread and want to have a look at how I solved generating regression variants, inspired by the original POC, I put my POC here Dynamic story variants POC. In the POC you specify what variants you want in the storybook setup phase. Then it will autogenerate the variants for each regression story automatically.

@shilman Thank you so much for taking the time to point me in the right direction! 👍

shilman commented 8 months ago

This is wonderful news @fredriknils and exactly what we were hoping to enable with the indexer API. At some point I'd love to find a general solution to build into core. But in the meantime it's great that you can hack it yourself. Kudos @JReinhold for making it happen!!

github-actions[bot] commented 7 months ago

Hi there! Thank you for opening this issue, but it has been marked as stale because we need more information to move forward. Could you please provide us with the requested reproduction or additional information that could help us better understand the problem? We'd love to resolve this issue, but we can't do it without your help!

github-actions[bot] commented 7 months ago

I'm afraid we need to close this issue for now, since we can't take any action without the requested reproduction or additional information. But please don't hesitate to open a new issue if the problem persists – we're always happy to help. Thanks so much for your understanding.

svallory commented 7 months ago

🥺 I just wasted hoooours trying to figure out why I wasn't getting the correct import path in Vite but instead the original file path.

My use case for this is simple: I'm creating a marko-stories addon to write stories in Marko. Using a virtual file path (like the example in the docs suggest) I could intercept the request in my plugin without interfering with Marko's vite plugin processing. I think this approach makes it really easy to add support for other languages. Here's an overview:

Add **/*.stories.marko pattern to stories
In the indexer I simply require the file so Marko compiler steps in
The components I created export the data I need to index the file
Set the import path to virtual:marko-stories:FILE_NAME
Handle the virtual file import in unplugin
extract the path and import it to let marko handle it again
grab the data exported by the components and build the CSF

Please, reopen this issue.

If @fredriknils (or anyone else) can point me to the fix location, I can make a PR

JReinhold commented 7 months ago

I think your use case is valid @svallory and I believe we designed the API to support exactly this kind of use cases. Looking at the code again it looks like a bug to me, we should use the importPath from the indexInput and not the "original" importPath as found on the fs. It especially does not make sense to ignore IndexInput.importPath given it's a required property.

I can make a PR

I believe the pointer in OP is correct, it should read from indexInput.importPath instead. I'm a bit unsure about the defaultMakeTitle function though...

https://github.com/storybookjs/storybook/blob/e942e71e1f5281f9197faab6d61625b0ac8cecfd/code/lib/core-server/src/utils/StoryIndexGenerator.ts#L302

svallory commented 7 months ago

Thanks for the quick reply, @JReinhold! I checked the code again and indeed that's the right place.

I've read the contribution documentation ~~, but couldn't figure out which branch to checkout (I imagine this is a small fix that will be released as a patch)~~

~~Should I checkout the latest-release branch?~~

Never mind, just checked the version on the code 🤦🏻

I'll run the tests and send a PR in a few moments

github-actions[bot] commented 6 months ago

Hi there! Thank you for opening this issue, but it has been marked as stale because we need more information to move forward. Could you please provide us with the requested reproduction or additional information that could help us better understand the problem? We'd love to resolve this issue, but we can't do it without your help!

tfoxy commented 6 months ago

Hi everyone! Yesterday I also run into this problem. I was following the example at the indexers doc, which uses importPath: 'virtual:jsonstories' to generate the stories in Vite.

I'm creating an addon similar to addon-svelte-csf, but with Vue. I'm not using storybook-vue-addon because I need to create more complex stories. The Svelte addon uses the following logic in Vite to create the story with CSF: vite-svelte-csf.ts

I did something similar with Vue as a workaround of not being able to use importPath. But it feels brittle: for example hot reload is not working properly because I had to replace the default export and I'm not sure how to fix that. With importPath it would be much simpler to implement the Vite plugin correctly:

return `
  import { addVueStoryDecorator } from "my-vue-addon";
  import __component, { meta } from ${JSON.stringify(storyPath)};

  export default addVueStoryDecorator(meta, __component);

  ${stories.map((story) => `export const ${story.name} = ${JSON.stringify(story.config)};`).join("\n")}
`;

Hopefully this can be fixed as it would make indexers more powerful. Thanks to the team for the work done with the indexers so far!

github-actions[bot] commented 5 months ago

Hi there! Thank you for opening this issue, but it has been marked as stale because we need more information to move forward. Could you please provide us with the requested reproduction or additional information that could help us better understand the problem? We'd love to resolve this issue, but we can't do it without your help!

github-actions[bot] commented 5 months ago

I'm afraid we need to close this issue for now, since we can't take any action without the requested reproduction or additional information. But please don't hesitate to open a new issue if the problem persists – we're always happy to help. Thanks so much for your understanding.

storybookjs / storybook