fix: preprocess svelte file during indexing

[ysitbon]

Closes #88

It fixes the issue where svelte files wasn't pre-processed during indexing making it impossible to load *.stories.svelte files with lang="ts" for example. Indeed this implementation is a very naïve approach. It walking up the directory tree to look for the first svelte.config.js file. It should solve 90% of the cases since svelte apps don't work without it. Indeed projects with exotic location or specifying a new svelte config in the storybook main file are not taken into account here.

Hope it helps

📦 Published PR as canary version: 2.0.12--canary.94.71b3707.0

✨ Test out this PR locally via:

npm install @storybook/addon-svelte-csf@2.0.12--canary.94.71b3707.0
# or 
yarn add @storybook/addon-svelte-csf@2.0.12--canary.94.71b3707.0

Version

Published prerelease version: v3.0.0-next.4

Changelog

💥 Breaking Change

• Require Storybook v7 in v3 #81 (@JReinhold)
• Story Indexer #77 (@JReinhold)

🐛 Bug Fix

• fix: preprocess svelte file during indexing #94 (@ysaskia)
• Support newest ESM-only @sveltejs/vite-plugin #84 (@JReinhold)
• Add vite support #72 (@IanVS)

Authors: 3

• Ian VanSchooten (@IanVS)
• Jeppe Reinhold (@JReinhold)
• Yoann (@ysaskia)

[JReinhold]

Thanks for this, this is great! 🙏

A few initial thoughts before I've tried it out.

This implementation is builder agnostic which is nice. But when looking at the implementation of loadSvelteConfig from @sveltejs/vite-plugin there's quite a lot of edge cases that it handles, that this PR doesn't. On one hand it would be nice to be builder agnostic. On the other hand, off-loading all the complexity to the official Vite plugin would make it easier to maintain here, and it would probably behave more predictively.

That would leave out Webpack users though. Is there a compromise where we somehow detect if this is a Vite or Webpack project, and use either the Vite plugin or this "naïve" approach in a Webpack project?

---

At a minimum I think we want to support svelte.config.mjs and svelte.config.cjs in addition to svelte.config.js though.

---

I'm not sure the whole findUp solution is necessary. At least in a Vite based setup, svelte.config.js always have to be in the CWD for the Svelte app to read it (not Storybook, but the actual app), and so I would think we can keep that same limitation here? I'm not sure if it's the same in a Webpack-based project though.

[ysitbon]

Hey!

Thanks for this precious feedback. I completely forgot to deal with the plethora of module loading possibilities! Regarding the link it's mostly what they're doing, the rest being related to Vite only (like resolving svelte config location from inline options or from root property set in Vite config).

IMHO I think that would be better to do it without calling Vite to keep this package builder agnostic. Not only for Webpack support but also for potential new builders (like Turbo?). Regarding the config loader I would proceed in 2 times:

1. Deliver our own solution to ship it asap. Mostly a copy / paste of what did Vite (without the parts related to Vite only). If agree with that I can easily add this to the PR and call it during indexing instead of findUp.
2. Then maybe try to deal with Svelte to have an agnostic config loader package with enough hooks that can satisfy everyone needing to load it manually. If no one wants to do it I would be volunteer too, but I would prefer to have it under @sveltejs.

Tell me what you think about it. If you're agree, in that case I would do it as soon as I can. Quite busy right now but pretty sure I can find some room to do deliver it very quickly.

[JReinhold]

I think that sounds like a good plan, looking forward to see your work!

If down the road users complain that the preprocessing doesn't match their Vite config, then we can do another pass at integrating deeper with that, but for now let's wait and see.

[ysitbon]

Here's a rewrite with the svelte config loader. I kept a variant of previous findUp() as a fallback but I'm not sure it's necessary. Also I throw an error when no config file. Maybe we don't want to be so restrictive, for me it does not make sense to work with Svelte without its config but maybe there is some good reasons to do it.

[JReinhold]

Tried it out, a few minor improvements needed, but otherwise great work!

[ysitbon]

Hey @JReinhold! Thanks a lot for the review. It is now updated accordingly. I used fs-extra.pathExists instead of native fs.exists since it's deprecated and fs-extra is already used in the package. Only one remark fs-extra is not in the package.json dependencies. Is this intentional?

[JReinhold]

Looks good! If you can apply my suggestion and add fs-extra as a dependency as it should be, this should be ready to go!

[JReinhold]

Thanks for your hard work @ysaskia ❤️

[shilman]

🚀 PR was released in v3.0.0 🚀