sveltejs / kit

web development, streamlined
https://kit.svelte.dev
MIT License
18.4k stars 1.88k forks source link

Proposal: Expose kit.vite in svelte.config.cjs #509

Closed GrygrFlzr closed 3 years ago

GrygrFlzr commented 3 years ago

Last proposed API from Rich:

module.exports = {
  kit: {
    vite({ mode }) {
      // mode is something like 'dev', 'build', 'service-worker'
    }
  }
};

Currently, #486 changed vite.config.js and the hardcoded Vite build step in order to better support the serverless platform usecase.

This approach of modifying the Vite config is quite restrictive and fragile:

Old preset proposal here A separate approach preact has taken is create a `withPreact()` function which takes advantage of `mergeConfig` and `UserConfig` from `vite`: - https://twitter.com/patak_js/status/1370480869325033473 - https://github.com/preactjs/preset-vite - Rationale: https://github.com/vitejs/vite/issues/2484#issuecomment-797702049 ```js import withPreact from "@preact/preset-vite"; export default withPreact({ // Your usual vite config }, { // Add your options here devtoolsInProd: true }); ``` Another possible suggestion in the Vite discord involved a factory method: ```js export default defineConfig([ usePreact({ devtoolsInProd: true }), { // Vite Config Options }, ]) ```

Unlike preact, we currently already have access to svelte-preprocess and the community-made svelte-adders, which are able to effectively add plugins like tailwind and mdsvex by only modifying svelte.config.cjs. However, I can imagine more complex situations where a vite plugin may be desirable. This is also more flexible than the snowpack config extending we did in the past.

Either way, this would allow us to update the config preset alongside kit, instead of having to tell users to migrate their config. This is especially important for the ssr field in the Vite config as the field is marked subject to change in the future.

I'm not entirely sure what the API should look like yet, but the preact examples above should be a good starting point.

Notable things to tackle:

antony commented 3 years ago

Name makes sense to me, since the preact one is prior art.

For the same reason the approach suggested also makes sense.

We get a fair amount of issues raised where people fail to update their config when migrating between versions of libraries - usually because they don't want to read the change log or migration notes.

Anything which reduces this burden therefore is good in my book.

dominikg commented 3 years ago

would this benefit from utilities provided by vite-plugin-svelte?

Note, in the next release automatic config of svelte depending on vite mode is going to be improved. no more need to specify hot or emitCss

rixo commented 3 years ago

Yes, yes!

Such a change would also be loved by tools like Svench, that want to programmatically augment / customize the user's config (to be able to build their components with the same plugins, preprocessors, etc., but by changing a few things like port and entry point). With the current Kit implementation, that would imply copying and maintaining the hardcoded config in Kit, with an unpalatable coupling between the tool (eg Svench) and Kit's versions.

Conduitry commented 3 years ago

I'm also +1 on this. In Sapper, requiring specific code in rollup.config.js/webpack.config.js was always a bit of a pain point, and made it hard to upgrade because of the need to manually merge sapper-template changes with your project's config.

Rich-Harris commented 3 years ago

At a high level I think this definitely makes sense. I'd suggest maybe @sveltejs/kit/vite-preset rather than a separate package, since the two things are likely to be very tightly coupled in terms of releases (and since it would contain Kit-specific stuff like aliases).

Where I can see it potentially getting tricky is around the service worker build, which needs to have an entirely different config to everything else.

benmccann commented 3 years ago

I slightly prefer the defineConfig syntax to the withPreact syntax. With withPreact I think it's less clear what goes in the first object vs the second object whereas with defineConfig it's much clearer how those two objects are being used

Rich-Harris commented 3 years ago

518 is another example where the shape of the config would potentially need to be somewhat different, which does make me wonder if exposing a preset is the right call versus our existing approach, which is nearly equivalent (except for the order in which config is applied — our config overrides user config, though that's probably for the best since all the options we pass directly from dev and build are necessary and shouldn't be overridden) but gives us more flexibility to vary it for different scenarios.

Rich-Harris commented 3 years ago

Put another way: a SvelteKit app isn't a Vite app, it's an app that uses Vite under the hood. (You can't run vite or vite build and expect it to do anything good.) So perhaps it would be preferable if most projects didn't have a vite.config.js at all, and either a) it was considered an optional thing for power users, or b) svelte.config.cjs exposed a way to modify the Vite config:

module.exports = {
  kit: {
    vite: ({ config, mode, ssr }) => {
      // mode is something like 'dev', 'build', 'service-worker'
      // config is what we currently pass to Vite programmatically
    }
  }
};

I've come to the view that it would only really make sense to have a preset approach if we rebuilt SvelteKit such that it were possible to do vite or vite build, but I'm not sure that's achievable.

antony commented 3 years ago

it's an app that uses Vite under the hood. (You can't run vite or vite build and expect it to do anything good.)

I hadn't realised that this was the case.

So perhaps it would be preferable if most projects didn't have a vite.config.js at all,

This shouldn't be the preference, it should be the rule. Having a vite.config.js would be hugely confusing, and there's no guarantee that anything you did in there would predictably, if at all. It's misleading.

or b) svelte.config.cjs exposed a way to modify the Vite config

This is what nuxt et al do. It's the right way to do it in my opinion (although when I used nuxt their modifier was all sorts of weirdness and black-box element find-and-merging. The above looks hugely simpler (maybe you could pass the entire config object as the last parameter to that method, for power users.

Rich-Harris commented 3 years ago

That sounds convincing. Given my earlier comments about the necessity of Kit-provided config not being overridden, perhaps the signature of config.kit.vite should be ({ mode }) => config, i.e. the user-provided function doesn't get to see/edit our config