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

Bring @sveltejs/kit/experimental/vite out of experimental #5184

Closed
benmccann opened this issue Jun 8, 2022 · 21 comments · Fixed by #5332
Closed

Bring @sveltejs/kit/experimental/vite out of experimental #5184

benmccann opened this issue Jun 8, 2022 · 21 comments · Fixed by #5332
Labels
feature request New feature or request vite
Milestone

Comments

@benmccann
Copy link
Member

benmccann commented Jun 8, 2022

Describe the problem

Right now SvelteKit asks users to pass Vite configuration via the kit.vite option in svelte.config.js. This was originally nice because it means less config files and gives SvelteKit a bit more control. However, it's also non-standard. Tools like Vitest and Storybook look for a vite.config.js to read configuration from and it would be nice to support those tools out-of-the-box

Describe the proposed solution

#5094 creates a SvelteKit plugin that can be imported in a vite.config.js. If a vite.config.js is created, it is expected that Vite config is no longer specified in svelte.config.js, but in vite.config.js.

This new feature can be used like:

// vite.config.js
import { sveltekit } from '@sveltejs/kit/experimental/vite';

/** @type {import('vite').UserConfig} */
const config = {
  plugins: [sveltekit()]
};

export default config;

Cleanup items

If we decide that we like this approach (and I think we probably will) then it would make sense to make some changes:

  • Make vite.config.js mandatory
  • Add it to project templates
  • Get rid of config.kit.vite
  • Move @sveltejs/kit/experimental/vite to @sveltejs/kit/vite or @sveltejs/vite-plugin-sveltekit
  • Update contributing guide with new code locations
  • Add vitest option to create-svelte
  • More controversially, we might want to get rid of svelte-kit dev/build/preview in favour of using vite dev/build/preview. This would mean that SvelteKit itself would no longer have a dependency on Vite, other than in the plugin; apps themselves would need to have a devDependency on vite
    • Consequently, figure out how to handle versions. It's likely that SvelteKit will expect a particular version of Vite, which might mean it needs to become a peerDependency if it's no longer a dependency (though if it's possible to avoid depending on a specific version, that would be ideal)

Alternatives considered

No response

Importance

nice to have

Additional Information

CC people who might be interested in this: @nickbreaton @JessicaSachs @IanVS @brittneypostma

Please feel free to share with others. I haven't used Storybook, Vitest, Histoire, etc. before so would love any help in testing! And would love tips on things we might want to document for the community to setup these tools. Whether there's something that needs to be improved or everything works, I'd love to hear about it either way!

@benmccann benmccann added the feature request New feature or request label Jun 8, 2022
@benmccann benmccann added this to the 1.0 milestone Jun 8, 2022
@benmccann benmccann added the vite label Jun 8, 2022
@brittneypostma
Copy link
Contributor

I think the storybook config is more of a pain because of Webpack config, not vite and SvelteKit. I can check this out though. Thanks for letting us know.

@dummdidumm
Copy link
Member

Does this mean that svelte.config.js is no longer used/required then? Asking because of the implications for language tools

@IanVS
Copy link

IanVS commented Jun 8, 2022

Storybook looks for a vite.config.js file when it is being bootstrapped into a project, to determine whether it should default to the webpack or vite builder. I've been trying to find a good way to detect sveltekit projects and default to the vite builder, since svelte.config.js is not a reliable indicator, I'm told. The vite builder itself doesn't read from any config file by default, so this change shouldn't cause any issues for us.

I think the storybook config is more of a pain because of Webpack config...

@brittneypostma can you explain a bit more what you mean by this?

@benmccann
Copy link
Member Author

Does this mean that svelte.config.js is no longer used/required then? Asking because of the implications for language tools

svelte.config.js will still be used and required. It just won't hold the Vite config. You'll also need a vite.config.js alongside it

I've been trying to find a good way to detect sveltekit projects and default to the vite builder, since svelte.config.js is not a reliable indicator, I'm told

svelte.config.js could be used by a Svelte project that is not a SvelteKit project. You could load the file and look for kit to be contained within the first-level configuration. However, that would not be as good as this solution because you'd be missing Vite config that SvelteKit sets up automatically such as path aliases. This changes moves that setup to occur within Vite plugins so that configuration will be made available by using the vite.config.js whereas it's not easy to get at today

@brittneypostma
Copy link
Contributor

Storybook looks for a vite.config.js file when it is being bootstrapped into a project, to determine whether it should default to the webpack or vite builder. I've been trying to find a good way to detect sveltekit projects and default to the vite builder, since svelte.config.js is not a reliable indicator, I'm told. The vite builder itself doesn't read from any config file by default, so this change shouldn't cause any issues for us.

I think the storybook config is more of a pain because of Webpack config...

@brittneypostma can you explain a bit more what you mean by this?

When installing storybook it uses Webpack and has it's own config file that needs to be set up and configured. That was the biggest pain for me in setting it up.

@IanVS
Copy link

IanVS commented Jun 8, 2022

When installing storybook it uses Webpack

Cool, yeah that's what we could avoid if sveltekit projects required a vite.config.js file as described here. Or if I implement another strategy as recommended above.

@benmccann
Copy link
Member Author

@sveltejs/kit/experimental/vite has been released in @sveltejs/kit version 1.0.0-next.353.

I tested with a simple Vitest project, but would love to have it tested in real projects as well as with other tools in the ecosystem like Storybook, etc.

@benmccann
Copy link
Member Author

CC @dimfeld @j3rem1e @TheComputerM as other people interested in SvelteKit Storybook support

@kyllerss
Copy link

I updated to SvelteKit-next.354 and reran my test case (vitest-dev/vitest#1520) and I'm still running into the same issue.

Is the process for testing this fix simply a matter of updating to next.354 or is there something else that needs to be done? I got somewhat lost in the comments above as to what was a migration change versus a proposed one. Thanks!

@benmccann
Copy link
Member Author

You need to create a vite.config.js file that imports and uses @sveltejs/kit/experimental/vite

@ZerdoX-x

This comment was marked as off-topic.

@benmccann

This comment was marked as off-topic.

@benmccann
Copy link
Member Author

benmccann commented Jun 28, 2022

For folks who would like to use Vitest, svelte-add-vitest now utilizes @sveltejs/kit/experimental/vite.

@ZerdoX-x

This comment was marked as off-topic.

@benmccann

This comment was marked as off-topic.

@IanVS
Copy link

IanVS commented Jun 29, 2022

FWIW, npm create svelte will create a project with the latest next version, but does not create a vite.config.js. I'm guessing it should, right?

@benmccann
Copy link
Member Author

FWIW, npm create svelte will create a project with the latest next version, but does not create a vite.config.js. I'm guessing it should, right?

No, it's still optional and not used by default until it's out of experimental. At that point we will require users to migrate. You can switch by creating a vite.config.js like the one in the issue description and removing any kit.vite config from the svelte.config.js file and putting it in vite.config.js instead

@michaelwooley
Copy link

Hi, I tried putting together a storybook example here: https://github.com/michaelwooley/storybook-experimental-vite

I was able to get storybook up and running with a few caveats (e.g. storybookjs/storybook#14952). I've added more detailed notes in the readme.

But it seems like I'm missing something: It reads like the introduction of a standalone vite.config.js is expected to make storybook setup simpler and reduce sharp edges? I'm still doing a lot of tinkering to get to a working setup. That makes me think that I'm over-complicating this in some way. Perhaps I'm misreading what is expected to happen from this change, though?

@IanVS
Copy link

IanVS commented Jun 30, 2022

@michaelwooley thanks for trying it out and for the well-documented example! The only thing that will change for storybook is that if you have a vite.config.js when you run npx sb init, we'll detect that file and set the core.builder option to @storybook/builder-vite automatically. You may also be able to use the sveltekit plugin in your viteFinal by merging it with the config.plugins, but I haven't tried that yet.

@michaelwooley
Copy link

@IanVS thanks for the info!

You may also be able to use the sveltekit plugin in your viteFinal by merging it with the config.plugins, but I haven't tried that yet.

Yes, I had many variations of viteFinal and one variation did pull in the sveltekit plugin. In retrospect, this is an important area to be tested. 🤦‍♂️

Added an issue for myself: michaelwooley/storybook-experimental-vite#5

Given that the scope of this issue is broader than just storybook, let me know if it would be better to discuss this somewhere else?

@benmccann
Copy link
Member Author

Thank you for providing the Storybook example! As an update for others following this thread, it works very well with a few changes. I sent a PR to do that: michaelwooley/storybook-experimental-vite#7. Thanks to @IanVS for providing some advice helping to get it setup!

Dilden added a commit to Dilden/helth that referenced this issue Jul 6, 2022
…e config options have been moved to vite.config.js
djavorek added a commit to szattila98/veryrezsi that referenced this issue Jul 7, 2022
djavorek added a commit to szattila98/veryrezsi that referenced this issue Jul 9, 2022
* Simple Dockerfile for client

* Client Docker image is Done & Neat

* Svelte installed as normal dep :(

No audit during build

* Moving compose out, .dockerignores

* Images workflow for Docker images & ECR

* Adding DEV.md to trigger build

* Updating aws region

* Setting up dependant CI jobs

* Depend on server test suite

* Fixing multi file COPY-ies

* Docker image publish only from main branch

* Some more to README, whitespaces

* Readme tweak

* Fixing a silly link in readme

* New packege-lock after merge

* One more readme link try

* Directly using Vite for build and package

sveltejs/kit#5184

* Adding node 18 workflow

Skipping workflows not related to the PR

* Remove test workflow config

omit=dev in client dockerfile
lovetodream added a commit to lovetodream/lovetodream.github.io that referenced this issue Jul 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request vite
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants