Normalize paths exposed to vite-builder's storybook-stories.js file

[Dschungelabenteuer]

What I did

Note The issue this PR addresses was reproduced on a Windows environment.

Context

I've finally found some time to upgrade from the 7.0 alpha. Because I am ahead of my time an impatient and curious lad, I did not target the latest stable release, but instead went directly for the latest pre-release.

The issue

Starting from 7.1.0-alpha.11, one of my Storybooks stopped working and raised the following errors:

Browser:

Uncaught SyntaxError: Invalid Unicode escape sequence

Vite server:

22:11:42 [vite] warning: 
/virtual:/@storybook/builder-vite/storybook-stories.js
1  |  const importers = {
2  |          './.storybook/docs/useAnything.docs.mdx': async () => import('/@fs/H:\Tests\vite-storybook-windows\.storybook\docs\useAnything.docs.mdx'),
   |                                                                       ^
3  |    './src/stories/Button.stories.ts': async () => import('/@fs/H:\Tests\vite-storybook-windows\src\stories\Button.stories.ts')
4  |      };
The above dynamic import cannot be analyzed by Vite.
See https://github.com/rollup/plugins/tree/master/packages/dynamic-import-vars#limitations for supported dynamic import formats. If this is intended to be left as-is, you can use the /* @vite-ignore */ comment inside the import() call to suppress this warning.

  Plugin: vite:import-analysis
  File: /virtual:/@storybook/builder-vite/storybook-stories.js

Where does it come from?

Notice how the above error displays non-normalized paths (path\to\file): this eventually causes errors since those single backslashes are interpreted as escaping characters.

• This probaby comes from Update glob to v10.0.0 #22171 which upgraded glob from version 8.x to 10.x
• Quite a jump forwards, since version 9 is a complete rewrite.
• Looking at the changelog, it looks like they introduced a lot of changes regarding Windows and escaping.

Basically, here's where the breaking difference seems to be coming from:

• 7.1.0-alpha.10 (glob@8.x) returns ['H:/Tests/vite-storybook-windows/.storybook/docs/useAnything.docs.mdx']
• 7.1.0-alpha.11 (glob@10.x) returns ['H:\\Tests\\vite-storybook-windows\\.storybook\\docs\\useAnything.docs.mdx']

I've came up with this quickfix, but:

• There might be a better way of fixing this (e.g. by passing options to glob)
• I only looked into the case I was facing, we might need to keep an eye on other use cases of glob within Storybook's codebase because there might still remain undetected related issues?

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

[JReinhold]

Thanks for a very thorough PR description, and great detective work!

I've looked through #22171 and it doesn't look like that upgrade could cause issues anywhere else - but I can understand why it would cause this issue.

Please give me some time to review this, as I have to set up Windows emulation on my MacBook first.

[JReinhold]

Looking at the glob documentation, it looks like there's a { posix: true } option that might also solve this problem instead of normalizePath from Vite. Could you try that out?

To be honest with you, I can't figure out which solution would be the most correct, so maybe it doesn't matter too much

[Dschungelabenteuer]

Hey Jeppe! Thanks for looking into this! No worries: I don't think this is urgent, I guess this just needs to be addressed before a full 7.1 release! I've just tried the posix option, here's the output:

As stated in the glob documentation:

• all paths are indeed converted back to forward slashes
• but they're also prefixed with //?/

Here's a summary:

1/ Current stories output:

['H:\\Tests\\vite-storybook-windows\\.storybook\\docs\\useAnything.docs.mdx']

2/ stories output using { posix: true }(*):

['//?/H:/Tests/vite-storybook-windows/.storybook/docs/useAnything.docs.mdx']

(*) This eventually raises an error: importers[path] is not a function (since path does not include the above prefix)

3/ Expected stories output (ensured by this PR)

['H:/Tests/vite-storybook-windows/.storybook/docs/useAnything.docs.mdx']

My 2 cents

It seems like both my current implementation and using the { posix: true } require us to map through the stories array. Performance-wise this should be pretty similar but:

• running normalizePath is probably slightly heavier than just simply getting rid of the //?/ prefix
• but in the mean time, it looks clearer/more self-explanatory because let's be honest, UNC paths do not feel quite natural.
• edit: also, despite normalizePath being possibly a bit heavier, relying on it might still be more flexible since it's tested on Vite's side and it could potentially support Vite-specific edge cases? (and we're in the builder-vite package). I'm not sure this argument is relevant, though :D)

Still open to go either way if needed :-)

P.S.: I re-requested a review just in case, please let me know if I should avoid doing that in the future! 😬

[JReinhold]

Thanks for taking the time to try it out! It definitely looks to me like we shouldn't use { posix: true }, as we'd still need normalizePath anyway.

[shilman]

@JReinhold does this need to be patched? the glob upgrade was not patched back to main

[JReinhold]

Great work! 💪 After spending many hours, I finally managed to get a development setup on Windows working, and can confirm this PR fixes the issue.

Thanks for your patience! 😅