Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SvelteKit: Create framework #20039

Merged
merged 19 commits into from
Dec 3, 2022
Merged

SvelteKit: Create framework #20039

merged 19 commits into from
Dec 3, 2022

Conversation

JReinhold
Copy link
Contributor

@JReinhold JReinhold commented Nov 30, 2022

This PR creates a dedicated framework for SvelteKit, based on the Svelte-Vite framework. 🚀

It currently does the same as Svelte-Vite, however in the future this will allow us to support SvelteKit specific features like the $lib module, etc.

Migration

The Svelte-Vite framework still supports SvelteKit, but I've added a deprecation warning telling users to migrate to this framework if SvelteKit is detected.

When do we want to remove this support completely in Svelte-Vite?
Some historical context for the decision:

So this deprecation really only targets SvelteKit users between v7.0.0-alpha.38 and v7.0.0-alpha.55 (2 months apart). I think we should remove support completely during 7.0 RC, considering the historical timeline, but I'm open to suggestions @shilman.

TODO

  • Migration notes
  • Generate repro when PR has been approved, but before merging (to not break CI)

matcherFunction: ({ dependencies }) => {
return dependencies.every(Boolean);
},
},
{
preset: ProjectType.SVELTE,
dependencies: ['svelte'],
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A stricter solution would be for this ProjectType.SVELTE's matcher function to not match if it sees the @sveltejs/kit dependency, but I can't figure out how this whole code works, so I can't make it work.

@@ -68,6 +68,7 @@ export default {
'@storybook/source-loader': '7.0.0-alpha.54',
'@storybook/store': '7.0.0-alpha.54',
'@storybook/svelte': '7.0.0-alpha.54',
'@storybook/svelte-kit': '7.0.0-alpha.54',
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shilman I don't know if this is legal to do. Or even if there's something other we need to do because this package is yet to be released.

@JReinhold JReinhold self-assigned this Dec 1, 2022
@JReinhold JReinhold added maintenance User-facing maintenance tasks svelte labels Dec 1, 2022
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome job!!

Are you open to changing the name from svelte-kit to sveltekit? It's what I called it in the frameworks post based on various googling.

Also, we should add an automigration for people upgrading from 6.x who are using SvelteKit. But we can do that in a later PR.

// Remove vite-plugin-svelte-kit from plugins if using SvelteKit
// see https://github.com/storybookjs/storybook/issues/19280#issuecomment-1281204341
plugins = withoutVitePlugins(plugins, ['vite-plugin-svelte-kit']);
// temporarily support SvelteKit until
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

until ... 😂

code/frameworks/svelte-vite/src/utils.ts Outdated Show resolved Hide resolved
@shilman shilman added feature request cli and removed maintenance User-facing maintenance tasks labels Dec 1, 2022
@JReinhold
Copy link
Contributor Author

Are you open to changing the name from svelte-kit to sveltekit? It's what I called it in the frameworks post based on various googling.

I see. The SvelteKit codebase is pretty inconsistent between the two, so I'm fine with either.

https://github.com/search?q=repo%3Asveltejs%2Fkit+sveltekit&type=code
https://github.com/search?q=repo%3Asveltejs%2Fkit+svelte-kit&type=code

@JReinhold JReinhold marked this pull request as ready for review December 1, 2022 13:23
@JReinhold JReinhold requested a review from shilman December 1, 2022 13:23
.vscode/settings.json Outdated Show resolved Hide resolved
@@ -574,6 +576,10 @@ If you were using `viteFinal` in 6.5 to simply merge in your project's standard

Previously, Storybook's Vite builder placed cache files in node_modules/.vite-storybook. However, it's more common for tools to place cached files into `node_modules/.cache`, and putting them there makes it quick and easy to clear the cache for multiple tools at once. We don't expect this change will cause any problems, but it's something that users of Storybook Vite projects should know about. It can be configured by setting `cacheDir` in `viteFinal` within `.storybook/main.js` [Storybook Vite configuration docs](https://storybook.js.org/docs/react/builders/vite#configuration)).

#### SvelteKit needs the `@storybook/sveltekit` framework

SvelteKit projects need to use the `@storybook/sveltekit` framework in the `main.js` file. Previously it was enough to just setup Storybook with Svelte+Vite, but that is no longer the case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't explain much what users have to do. Are you planning to add an automigration? See this as reference

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes

@yannbf yannbf requested a review from benmccann December 2, 2022 09:43
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good!

@@ -576,6 +578,10 @@ If you were using `viteFinal` in 6.5 to simply merge in your project's standard

Previously, Storybook's Vite builder placed cache files in node_modules/.vite-storybook. However, it's more common for tools to place cached files into `node_modules/.cache`, and putting them there makes it quick and easy to clear the cache for multiple tools at once. We don't expect this change will cause any problems, but it's something that users of Storybook Vite projects should know about. It can be configured by setting `cacheDir` in `viteFinal` within `.storybook/main.js` [Storybook Vite configuration docs](https://storybook.js.org/docs/react/builders/vite#configuration)).

#### SvelteKit needs the `@storybook/sveltekit` framework

SvelteKit projects need to use the `@storybook/sveltekit` framework in the `main.js` file. Previously it was enough to just setup Storybook with Svelte+Vite, but that is no longer the case.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
SvelteKit projects need to use the `@storybook/sveltekit` framework in the `main.js` file. Previously it was enough to just setup Storybook with Svelte+Vite, but that is no longer the case.
SvelteKit projects should use the `@storybook/sveltekit` framework in the `main.js` file. Previously it was enough to just setup Storybook with Svelte+Vite, but that is no longer the case.
\`\`\`js
export default {
framework: '@storybook/sveltekit',
};
\`\`\`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm okay with this suggestion, but you should know that Storybook is unusable if SvelteKit isn't handled explicitly like the framework does. Do you still think it should be changed to "should"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then "need to" or "must" WFM

@shilman shilman merged commit 5e6445f into next Dec 3, 2022
@shilman shilman deleted the jeppe/sveltekit-framework branch December 3, 2022 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants