Passing the Rollup output options through to Vite

[johnnysprinkles]

This allows us to have more control of the rollup chunking when doing a production build. In particular, it allows us to specify manualChunks so we can decide what goes in the vendor chunk, or to not have a vendor chunk at all and simply have large page-specific bundles.

vite.build() does occur in three places in that file, I only updated in the one place, and verified it took effect when doing an "npm run build". Not sure if I should also update in the other places.

Fixes #1571

Before submitting the PR, please make sure you do the following

• It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
• This message body should clearly illustrate what problems it solves.
• Ideally, include a test that fails without this PR but passes with it.

Tests

• Run the tests with pnpm test and lint the project with pnpm lint

Changesets

• If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpx changeset and following the prompts

[johnnysprinkles]

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.

[johnnysprinkles]

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.

[Rich-Harris]

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.

[johnnysprinkles]

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.

[johnnysprinkles]

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

[johnnysprinkles]

Here's with "new Error":

[johnnysprinkles]

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.

[johnnysprinkles]

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

[johnnysprinkles]

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.

[johnnysprinkles]

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.

[benmccann]

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

[johnnysprinkles]

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.)

[johnnysprinkles]

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).

[johnnysprinkles]

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

[benmccann]

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

[benmccann]

could we put the print_vite_config_conflicts method somewhere shared and use it here?

[johnnysprinkles]

Cool, how about a generic "print_config_conflicts" that might as well use the logger() I suppose. See update.

[benmccann]

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

[changeset-bot]

⚠️ 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.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

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

[johnnysprinkles]

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

[johnnysprinkles]

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

[benmccann]

thanks for all your work on this one!

[johnnysprinkles]

Thanks for the review! I’m on a road trip from Seattle to Palm Springs and did bring my laptop for more revisions but also cool to just get it in there.

…

On Mon, Jun 14, 2021 at 8:42 AM Ben McCann ***@***.***> wrote: thanks for all your work on this one! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1572 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHYF4WVWEAJGEZ7XS3WJS43TSYPPDANCNFSM45VJUBUA> .