Actually, wouldn't it make sense to do the same change in build_server and build_service_worker as well? For people not providing any rollupOptions.output config, it has no effect on them. I can go ahead and update the PR if so.

Would it actually make more sense to fix this in a general way, making a simple deepMerge() function for example and using that to recursively merge the user config with SvelteKit's own Vite config? Definitely not suggesting we take on a new dependency just for a merge function, we can simply write one.

Huh, that seems like a weird default on Vite's part. I'm sure there's a rationale I'm not aware of. I suspect we should be overriding that by default, regardless of whether we respect the user's configuration here.

I think the general deepMerge solution probably makes sense, yeah. I think it would be good if we took that opportunity to explicitly warn if the user is setting leaf properties that are getting overridden by SvelteKit — e.g. in a case like this...

// svelte.config.js
export default {
  vite: {
    build: {
      outDir: 'custom-out-dir'
    }
  }
};

...we might print a loud warning saying something like

The value for vite.build.outDir specified in svelte.config.js has been ignored, as this is controlled by SvelteKit

We might need some way to indicate that a property shouldn't be merged. For example while we want to merge plugins...

plugins: [
  ...(user_config.plugins || []),
  svelte({
    extensions: config.extensions,
    emitCss: !config.kit.amp
  })
]

...we don't want to respect the user's optimizeDeps.entries since there's no sensible value the user could supply there. Not 100% sure what that looks like.

Actually, wouldn't it make sense to do the same change in build_server and build_service_worker as well?

Generally speaking it's very possible that some options make sense in one context but not in another. We sort of anticipated this by making config.kit.vite a function (objects get turned into functions)...

const user_config = config.kit.vite();

...which could in theory receive options...

const user_config = config.kit.vite({ type: 'service-worker' });

...but that's a bit of a digression and we probably don't need to worry about that right now.

I think Vite just got that from the Rollup docs where they give "vendor chunk" as an example.

But so added a deep_merge function to core/utils.js. Not sure if it should go there, or maybe it would be better as a separate "merge.js" file in the same location.

I was just going to do objects and let everything else be atomic, but the way the vite.plugins merge together as an array made me think it should be 1. Objects interleave together, 2. Arrays combine, 3. Anything else just overwrites.

For the error message, I started with new Error but I guess it shouldn't be a fatal error, so I did console.error but that wasn't colorfully highlighted, so I ended up with console.error(colors.bold().red(...)). I'll drag a couple screenshots in.

Also not sure where the tests should go for deep_merge, I just put along side.

Here's with "new Error":

And here's with console.error(colors.bold().red()). Not sure if we need the prefix saying which function it happened in, but each build() function does have different SvelteKit config for Vite.

Maybe it should show the whole path, kit.vite on down, and maybe format that part of the error somehow? Here's in italic:

And, this is a pretty significant change and somewhat of a regression risk. We can always just do that first commit instead, mixing it in surgically.

So I was concerned about this recursive merge because it recurses down through objects that might not be "plain old JS objects" but may have stuff in closure, or rely on this, etc. But after a bit of investigation I'm less concerned now. If they rely on values in closure that will still be there if you copy all the properties. Maybe there could be this issues, but that would be easy to fix by checking for "plain old JS objects" i.e. objects where the constructor === Object. So instead of:

const is_object = (x) => typeof x === 'object' && !Array.isArray(x);

Might be more like:

const is_plain_object (x) => typeof x === 'object' && !Array.isArray(x) && x.constructor === Object;

Happy to make this change if desired. Plugins are certain to be fine already, recursion stops at arrays so they never get deep cloned.

@johnnysprinkles thanks for this! as a heads up, there's quite a few errors being thrown by pnpm check

Thanks for taking a look @benmccann, yep getting up to speed on the process here and fumbling through the jsdoc typing. I'll have an update later today addressing your comments and passing all the checks. (My "today" being US west coast time.)

I was pretty out of date, synced my fork and all should be good now. The dev part looks like this in iTerm2 (annoyingly it won't say what color scheme I have enabled but it's a standard one).

Oh, that's "The Hulk" from https://iterm2colorschemes.com/, one of my favorites.

@johnnysprinkles it looks like this one will need to be rebased

Cool, this is looking pretty good! It will still need to be rebased

The other thing I was thinking is that utils is kind of becoming a dumping ground and maybe we can organize it a little better. One idea would be to rename the load_config directory to config and have it export a load function as well as the new functions that you're adding in utils

⚠️ No Changeset found

Latest commit: 7e8e4ef

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

A little messy but makes sense if you view the diff of all commits as a whole.

And yeah, I agree about load_config => config, makes sense to me.

thanks for all your work on this one!