Fix experimental_indexers importPath being ignored

[svallory]

Closes #25554

What I did

Added a simple "or" to try to use indexers.importPath but fallback to the original file path.

Couple of things to note

1. importPath is currently required. I think it is safe to make it optional with the default being the path of the file being indexed. I haven't added a test case because of this.
2. Contribution guide didn't mention anything about bumping the version, so I didn't

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

• stories
• unit tests
• integration tests
• end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

• Add or update documentation reflecting your changes
• If you are deprecating/removing a feature, make sure to update
  MIGRATION.MD

Checklist for 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:

  Available labels

  • bug: Internal changes that fixes incorrect behavior.
  • maintenance: User-facing maintenance tasks.
  • dependencies: Upgrading (sometimes downgrading) dependencies.
  • build: Internal-facing build tooling & test updates. Will not show up in release changelog.
  • cleanup: Minor cleanup style change. Will not show up in release changelog.
  • documentation: Documentation only changes. Will not show up in release changelog.
  • feature request: Introducing a new feature.
  • BREAKING CHANGE: Changes that break compatibility in some way with current major version.
  • other: Changes that don't fit in the above categories.

🦋 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>

[svallory]

@JReinhold when you have some time, please take a look at the questions I had

[svallory]

Hey @JReinhold, I got my storybook sandbox working yesterday to test this PR and there was a piece missing. The builder-vite package also needed to be updated to handle the importPath correctly.

Initially I thought of importing StoryIndexGenerator from core-server but I quickly ran into issues related to esm vs cjs. So instead I assumed that a custom importPath provided by an indexer will ALWAYS be virtual. I think that's

Similar problems may arise for other builders. Since I'm still learning about the inner workings of Storybook, it would be great if someone from the core team could take at this. It would take me quite some time to verify since I would still need to setup sandboxes for each builder.

If someone finds and issue with a builder and points me to the right location, I can quickly fix other builders applying the same logic.

After I've received an approval for the implementation, I'll add tests, an importPath validation, and a note to the docs to inform of the importPath requirements. For example, that the importPath needs to be either the original path or a virtual module path following vite/rollup convention (if that's what we decide)

[svallory]

crap... @ndelangen I wanted to pull and did the opposite 🤦🏻 and I just realized it's too easy to do a force push with Lazygit.

I noticed there was a commit from you before I did this atrocity. Was it just a rebase on top of latest-release?

[svallory]

I've rebased the branch on top of latest-release and added a commit that fixes the ExtractPresetType (renamed to UnwrapPresetType) and (hopefully) all the build issues

[svallory]

Hey @JReinhold or @valentinpalkovic, it looks to me that what we need now is to update the snapshots due to the change of the import path. But I think it is better if someone from the core team take a look at it

[JReinhold]

So instead I assumed that a custom importPath provided by an indexer will ALWAYS be virtual.

Can you tell me more about this, why do you assume this?

[JReinhold]

I'm seeing lots of changes here unrelated to importPath - mostly preset handling and typing. Are they a result of a bad rebase/merge or are all these changes intentional?

Either way I think we should keep them out of this PR.

[svallory]

@valentinpalkovic I rebased my branch on top of next and just pushed the fix for indexer's importPath. I also updated the affected tests snapshots.

Note:

• the tests that are failing were already failing on a clean checkout of the next branch
• the importPath returned by indexers is an absolute path (I don't know if that is an issue). I think it must be used as is since importers need to be able to return virtual paths, and it would be very hard to figure out if the path can be made relative to the project/source root or not.

[svallory]

Btw, @valentinpalkovic I accidentally committed a new sandbox template for Marko (writing stories in Marko is the whole reason I'm sending this PR).

I'll remove it in the next commit.

Would a PR to add Marko JS as a sandbox template and fully supported framework acceptable?

[JReinhold]

Great cleanup, much easier to review now. 😅 Still a couple of questions and feedback points to touch on.

Could you fill out the "Manual Testing" section in the PR, with steps on how to test this? Just something minimal, with a custom indexer returning a custom virtual importPath, and a very minimal Vite plugin that adds that virtual module with some CSF content in it.

I'm sorry if it feels like I'm nitpicking here, but you're touching on some of the most essential code in Storybook, and breaking it could cause issues for many users. We'll get there! 😊

[svallory]

I'm sorry if it feels like I'm nitpicking here, but you're touching on some of the most essential code in Storybook, and breaking it could cause issues for many users. We'll get there! 😊

Hey, @JReinhold, don't worry about it! I wanna get this right! ;)

I was actually trying to get a sandbox for Marko to test this fix, but I ran into to a lot of issues while trying to do that.

Can you help me get that up and running? I think it would become an awesome example of customization since it will touch on all that one can do using custom indexers. In trying to fix it, I started adding Marko alongside all the other frameworks in every place I could find, and it feels like I'm close now.

I've pushed my WIP to the feature/marko-framework of my fork and created a PR to this branch to make it easier for you to check what changes I did. The marko-stories code isn't there, I added the @storybookjs/marko repo as a submodule in code/marko to make it easier to create a sandbox.

It would be great to use the add-on I'm already creating as the manual test. But if this is all too much work, let me know and I'll come up with something else.

I intend to work on this tomorrow to address all the comments you've made.

[JReinhold]

I've pushed my WIP to the feature/marko-framework of my fork and created a PR to this branch to make it easier for you to check what changes I did. The marko-stories code isn't there, I added the @storybookjs/marko repo as a submodule in code/marko to make it easier to create a sandbox.

I quickly looked through it, looks straightforward and you are touching the right files, but I don't think the git submodule idea will hold. I suggest you draw inspiration from the Solid and Qwik integrations, which are both examples of frameworks+renderers that are initable and sandboxable but are actually not in the monorepo. I think there are some limitations with this, but for now this the best we can do.

https://github.com/storybookjs/storybook/blob/next/code/lib/cli/src/sandbox-templates.ts/#L492-L504

https://github.com/storybookjs/storybook/blob/next/code/lib/cli/src/generators/QWIK/index.ts

https://github.com/storybookjs/storybook/blob/next/code/lib/cli/src/project_types.ts/#L23-L26

It would be great to use the add-on I'm already creating as the manual test. But if this is all too much work, let me know and I'll come up with something else.

We use the Manual Test section to manually verify things work before we to minor/major releases (in about 2 months). The best way to do that are with minimal steps, similar to a minimal reproduction. It's just more fool proof if the manual steps are minimal, and don't depend on bigger things. If things fail it could be impossible to tell if they fail because of some bug in the Marko integration or because there's an issue with the import paths, and I want to avoid that confusion if I can.

[svallory]

I quickly looked through it, looks straightforward and you are touching the right files

That's great! I was concerned I was doing way too much just to support sandboxing.

I don't think the git submodule idea will hold

That's just to make my exploration easier. I didn't intend to propose it.

suggest you draw inspiration from the Solid and Qwik integrations, which are both examples of frameworks+renderers that are initable and sandboxable

Great! That makes it way easier.

The best way to do that are with minimal steps

I agree. This should not be too hard. In fact, I think I should also add a unit test for indexers returning virtual paths. And a note to the docs about it (since there's a recommendation from vite/rollup regarding their syntax). I think I'll take inspiration on the JSON stories from the docs and make an example that embeds the stories file content in a virtual path using base64. That will eliminate the fs loading and parsing in vite which can be cumbersome to do at runtime.

One more thing...

I noticed your comment about the dynamic snapshot. That was the easiest path to make sure everything was working. The only way I see out of that is to make the path relative again. We can use the StoryIndexGenerator.options.workingDir for that.

Also, we can do this in the generator's extractStories() function, but that wouldn't work for all tests or all paths.

The only way to ensure the removal of machine-specific paths in the return would be to process the generator's result in the tests before comparing it to the snapshot.

Please let me know how to proceed. I'll work on the final touches tonight/tomorrow

[svallory]

@tmeasday

I don't understand why there would be absolute paths in the index.

The reason it is there is that Storybook passes an absolute path to the importer. Since the test importers simply return the path, the absolute path gets into the index.

This seems like it shouldn't happen, and is a security issue.

That was my first reaction too, but if you think about it, there's nothing preventing the indexer code of reading the entire filesystem if it wants to. AFAIK, unless Storybook implements a require/import handler to override node's fs module (if that's even possible), that will remain to be the case, right? Or is there something I'm missing here?

I do feel it is kinda odd to deal with absolute paths, in a web app. But may get complicated on that front when you factor in all the variables involved. If we do relativize the path, what will be used as the base for import? The project root, the build/web root, the custom indexer file? When the bundler plugin for the indexer gets the file, will it get the relative or the absolute path? If it's a relative path, can we make sure the cwd when the indexer's bundler plugin is the same as the base path used to relativize the story path?

What I'm trying to point out here is that there are a lot of things to think about when forcibly modifying the indexer's return. This is a decision that the Storybook team will need to discuss and take very carefully.

I think it is outside of the scope of this PR though. The goal here is just to make indexers work as advertised in the docs. Right now, what is documented in the site does not work. It never has. I hope the fact the the indexers are experimental is enough to allow us to move on and change the path behavior (if that's the decision) in another PR

I'll be pushing the fixes for all other issues later today!

[tmeasday]

If we do relativize the path, what will be used as the base for import? The project root, the build/web root, the custom indexer file?

The CWD is what we have been using up until now.

Basically the import path is going to be relative to the virtual module that defines the importFn and does the actual import. And we've been adding that virtual module at the CWD (here's how webpack does it - I'm less sure about Vite).

This isn't really something the indexer can change -- it isn't the thing doing the importing so should have to fit with the existing import() behaviour.

So I'd propose:

• if it passes a relative path it is assumed to be relative to the CWD
• if it passes an absolute path it relativized to the CWD.

I think it is outside of the scope of this PR though. The goal here is just to make indexers work as advertised in the docs. Right now, what is documented in the site does not work. It never has. I hope the fact the the indexers are experimental is enough to allow us to move on and change the path behavior (if that's the decision) in another PR

I'm not really too concerned about the behaviour of customer (experimental as you say) indexers, I am concerned about the default behaviour in Storybook as exhibited by the tests. Am I off base about that?

[svallory]

it isn't the thing doing the importing so should have to fit with the existing import() behavior.

I think you are right. As long as we only have imports of real files in the importFn making the paths relative should not cause any issues. At least not now, since there's no plugin relying on that path. So it is better to make this change now, before people start using it.

I am concerned about the default behaviour in Storybook as exhibited by the tests. Am I off base about that?

Not at all! What I meant was that Storybook isn't introducing any security risks by exposing the absolute path to the indexer. But that's because I assumed that the code generated for those imports was only used in dev mode. If that's not the case, or may not be the case in the future, then we would be exposing paths to the web that may indeed aid an attacker.

[tmeasday]

If that's not the case, or may not be the case in the future, then we would be exposing paths to the web that may indeed aid an attacker.

Right, so yes, the index.json ("stories json") is produced in production builds of SB so it would be a big problem to have absolute paths in there. Honestly I think folks would find it pretty disconcerting if they were in there in dev mode too (and as it happens the index.json should be the same in both modes anyway).

[svallory]

If that's not the case, or may not be the case in the future, then we would be exposing paths to the web that may indeed aid an attacker.

Right, so yes, the index.json ("stories json") is produced in production builds of SB so it would be a big problem to have absolute paths in there. Honestly I think folks would find it pretty disconcerting if they were in there in dev mode too (and as it happens the index.json should be the same in both modes anyway).

I understand your concern. And yeah, even in dev mode, people would complain about it.

So, I stumbled on an issue here. How should we deal with virtual paths that look like absolute paths?

Check this example from [@storybook/builder-vite](export const virtualStoriesFile = '/virtual:/@storybook/builder-vite/storybook-stories.js'):

export const virtualStoriesFile = '/virtual:/@storybook/builder-vite/storybook-stories.js';

Right now, it would be resolved to something like this...

path.relative(process.cwd(), '/virtual:/@storybook/builder-vite/setup-addons.js')
'../../../../../../../../../virtual:/@storybook/builder-vite/setup-addons.js'

We can enforce a specific prefix for virtual paths (traditionally virtual:pkg-name), and informed the user of the requirement on every import error "in case they are trying to use a virtual path.

Or we could add a new property called isVirtual to the IndexEntry interface. Usually I would prefer this option, but it doesn't look like easy to propagate that all the way to the importFn code generator.

What do you guys think, @tmeasday and @JReinhold ?

[tmeasday]

Check this example from @storybook/builder-vite
export const virtualStoriesFile = '/virtual:/@storybook/builder-vite/storybook-stories.js'

@svallory but that path wouldn't end up in the index.json right? Is there a scenario you can think of where an indexer would ever create a virtual path for a story file? If not, I'd say we could just not allow it and throw if you pass a path that isn't either a simple relative or absolute path.

[JReinhold]

Is there a scenario you can think of where an indexer would ever create a virtual path for a story file?

I believe that's actually the point of this whole exercise @tmeasday , and the only use case for indexers to specify their own importPath. (We explored this topic further up in the PR but I understand it's hard to keep up with this)

If you have an input file - allFixtures.json that you want to split up into multiple components, the best way to do that is to generate a virtual module for each component, that the indexer then sets as the importPath.
I'm not sure there's any other use case for setting an importPath different from the original

We can enforce a specific prefix for virtual paths (traditionally virtual:pkg-name), and informed the user of the requirement on every import error "in case they are trying to use a virtual path.

Vite already establishes a convention for this with the virtual: and \0virtual: prefix and I think we should just lean into that. If the importPath starts with either fo those, consider it a virtual module. If not, assume it's a file path and convert it to a relative path if needed.

https://vitejs.dev/guide/api-plugin#virtual-modules-convention

[svallory]

I believe that's actually the point of this whole exercise @tmeasday , and the only use case for indexers to specify their own importPath.

That's exactly right. In my case I'm creating an interceptor between Marko and Storybook to allow stories to be written in Marko. I can mess with the files, otherwise I'll break Marko's build.

I actually tried (a lot) to get around the need for a virtual path, but not matter what I did, at one point or another, things would go haywire. Vite simply isn't very reliable or predictable when it comes to plug-in execution :(

Vite already establishes a convention for this with the virtual: and \0virtual: prefix and I think we should just lean into that.

That's my recommendation too. But just to give credit where credit is due, that mechanism and convention was established by Rollup ;)

(Sorry, I guess I'm a bit bitter that Vite become the standard and now I have to endure it hahaahaha but seriously... I've lost countless hours because of it)

[tmeasday]

Ok, thanks for filling me in (and sorry for not reading the whole thread). That approach sound sensible to me!

[dunklesToast]

just came across this thread because i think this would also solve an issue i am currently facing. would it be possible for somebody from the @storybookjs/core team to create a canary release for pr so i can check if this works for my usecase as well or if i need to dig somewhere else?

[storybook-bot]

Failed to publish canary version of this pull request, triggered by @valentinpalkovic. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/9356213289

[storybook-bot]

Failed to publish canary version of this pull request, triggered by @valentinpalkovic. See the failed workflow run at: https://github.com/storybookjs/storybook/actions/runs/9356213289

[nahtnam]

Same here, happy to test a canary. Thank you @svallory for working on this

[svallory]

Quick update:

Sorry for being MIA. I'm working on a big launch due next week and have no time at all to work on anything else. If anyone wants to take this over the finish line, please go ahead! There isn't much left to be done.

I'm available to answer any questions. Just send me a DM on twitter @svallory_en or discord

[dunklesToast]

I'd love to take over but after reading the thread I am not 100% sure whats left to be done here? Probably fixing the TypeScript types that break the build right now and some manual testing?

[svallory]

I'd love to take over but after reading the thread I am not 100% sure whats left to be done here? Probably fixing the TypeScript types that break the build right now and some manual testing?

Hey @dunklesToast, that's great! There isn't much left to do. IIRC, all that is left is:

• Make sure absolute paths are turned into relative ones
  This will affect StoryIndexGenerator.ts and maybe can be entirely solved there.
  For context, check these comments:
  • #issuecomment-2135635954
  • #issuecomment-2136405529
  • #issuecomment-2136852474
  • #issuecomment-2137047417

Note

Be aware of Rollup's virtual module convention for virtual modules adopted by vite:

• Updating the tests accordingly
• Create a simple sandbox example that displays the use of experimental indexers.
  The documentation has some examples at the end that you may use so you don't have to create one from scratch.
  Just make sure the path you are using to load the stories is a virtual one or, at least, not the original path