refactor: opt-in optimizeDeps during build and SSR

[patak-dev]

Description

We still have some open issues with deps optimization during build time and SSR, and this is the last thing blocking v3:

• Tree shaking isn't working correctly in some scenarios
• During SSR, we are changing how the heuristic works and adding the ssr.optimizeDeps options days away from the release
• We need more testing to see how the move away from @rollup/plugin-commonjs affects performance, bundling size, and stability in general

We were pushing hard to advance with this change because we thought that Vite v4 was a year away.
Now that rollup v3 will be out soon, we may release Vite v4 in a few months, so we are thinking of changing our strategy.

So, given that:

• The migration from v2 -> v3 is already quite beefy: https://main.vitejs.dev/guide/migration
• We need more time to remove plugin-commonjs without disruption
• We may release Vite v4 in a few months

We are thinking that we should:

• Remove legacy.buildRollupPluginCommonjs and change the defaults to:
  • optimizeDeps.disabled: 'build'
  • ssr.optimizeDeps.disabled: true

You would still be able to enable optimization for build time, or SSR as opt-in and, and independently set build.commonjsOptions: { include: [] } to remove the commonjs plugin. We can then work to test and iron out this during the next weeks/months, and flip the defaults in Vite v4.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[netlify]

✅ Deploy Preview for vite-docs-main ready!

Name	Link
🔨 Latest commit	d65e896
🔍 Latest deploy log	https://app.netlify.com/sites/vite-docs-main/deploys/62c7ddbdbed7840009eab1b3
😎 Deploy Preview	https://deploy-preview-8965--vite-docs-main.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site settings.

[patak-dev]

@bluwy looks like the import assert test doesn't work with plugin commonjs. For now, I enabled deps optimization for that playground.

[bluwy]

@bluwy looks like the import assert test doesn't work with plugin commonjs. For now, I enabled deps optimization for that playground.

👍 I think that's fine for now, unless we start to strip import assertions in node_modules in build too, which is doable, but might have an impact on perf.

[brillout]

Seems good from my perspective 👍. It's only a feature flag away, so it's a safe move anyways.

And this doesn't affect our new approach of externalizing SSR deps by default, correct? (I believe this change to be quite important and I think it should, ideally, be shipped in v3; happy to elaborate if needs be.)

(Little thing: I'm probably being naive here, but I wonder if this change could be introduced in a Vite 3 minor: I mean the goal here is to stay backwards compatible, so a minor seems fitting. I'm even wondering whether Rollup v3 could be introduced in a minor: Rollup v3 is certainly a breaking change for Vite but not necessarily for the user actually no since the user can touch the Rollup config, so that's a breaking change for the user as well. Feel free to not reply to any of this parenthesis; I'm just thinking out loud here.)

[patak-dev]

Seems good from my perspective 👍. It's only a feature flag away, so it's a safe move anyways.

Yes, and we would work with framework maintainers to turn this one automatically as issues are resolved. We already saw from this past month's experience that the feature works quite well.

And this doesn't affect our new approach of externalizing SSR deps by default, correct? (I believe this change to be quite important and I think it should, ideally, be shipped in v3; happy to elaborate if needs be.)

SSR is still ESM and all deps are external by default in v3, yes.

(Little thing: I'm probably being naive here, but I wonder if this change could be introduced in a Vite 3 minor: I mean the goal here is to stay backwards compatible, so a minor seems fitting. I'm even wondering whether Rollup v3 could be introduced in a minor: Rollup v3 is certainly a breaking change for Vite but not necessarily for the user actually no since the user can touch the Rollup config, so that's a breaking change for the user as well. Feel free to not reply to any of this parenthesis; I'm just thinking out loud here.)

I don't think we need to do this in a minor if the next major is only a few months from now, and most users would experience vite through a framework (and each framework can already flip the default at their own pace)

[aleclarson]

I approve, except I would rename the options to build.optimizeDeps: false and ssr.optimizeDeps: false.

Reason being it feels weird to use disabled: false to opt-in to a feature.

[patak-dev]

I approve, except I would rename the options to build.optimizeDeps: false and ssr.optimizeDeps: false.

Reason being it feels weird to use disabled: false to opt-in to a feature.

This was the first API we tried when we wanted to add the possibility to disabled the optimizer, but we had to backtrack as it breaks the way the ecosystem is using the config field. For example optimizeDeps?.include isn't possible anymore. So we already added optimizeDeps.disabled: true. And it was disabled, because by default it is enabled. As you said, it is weird that an opt-in feature uses this API but if things go well we are going to flip the default so I think we can keep the current API.

[patak-dev]

We discussed this change with the team and other maintainers in the ecosystem, and we think we should proceed. It should be an easier jump for users if we push this to v4, and it gives us more time to review perf and stability of esbuild deps optimization during build time.

[nickserv]

When would it be a good idea to test using disabled: false? Does this improve performance since esbuild is used?

[bluwy]

It would be experimental if you want to enable optimizeDeps in build in Vite 3, so any help with testing it would be appreciated, but also expect bugs if they come by. It should improve build performance if you have many dependencies, and should reduce code inconsistency between dev and build.

[jiby-aurum]

@bluwy @patak-dev I was testing lib build with optimiseDeps opt in. The build is much faster but also seemingly it makes treeshaking work, which otherwise was not working resulting in almost double bundle size.

However a strange behaviour I notice is for the first build, when the cache was not available, the bundle is a bit larger (for my project 10M for first build and 9.7M for subsequent ones). Any clue why this is happening ?

[jiby-aurum]

okay, I diffed the bundles to see what's different, and seemingly its just naming of variables created during the bundle, the first bundle has $1 appended with those variable names whereas subsequent builds does not have it. See a section of the diff below

[patak-dev]

@jiby-aurum thanks for testing! Would you create an issue with a small reproduction of this problem? So weird... it should be using the same bundles in both cases. PR welcome too if you have some time to dig into the code.

[jiby-aurum]

@patak-dev will do, as soon as I can find some time :)

[patak-dev]

Thanks!

[jiby-aurum]

@patak-dev sorry did not yet find enough time, for what I promised above

But I discovered another issue in ssr mode for the dep optimiser. In case of cjs dependencies that require node built-in modules the default exports seems to be not handled properly.

A minimal repo case would be a dependency lets say sample-dep that does

const EventEmitter = require('node:events')

export default class extends EventEmitter {}

which for example is consumed by code being built like so

import Test from 'sample-dep'

export function test() {
   return new Test();
}

with the vite configuration below,

import {defineConfig} from "vite";
import module from 'module'

export default defineConfig({
    ssr: {
        external: module.builtinModules,
        noExternal: true,
        optimizeDeps: {
            disabled: false
        }
    },
    build: {
        ssr: true,
        commonjsOptions: {
            include: [],
        },
        rollupOptions: {
            input: './index.js',
            external: module.builtinModules,
        }
    }
})

it generates as output

import { createRequire } from "module";
import * as events_star from "events";
createRequire(import.meta.url);
var __defProp = Object.defineProperty;
var __getOwnPropDesc = Object.getOwnPropertyDescriptor;
var __getOwnPropNames = Object.getOwnPropertyNames;
var __hasOwnProp = Object.prototype.hasOwnProperty;
var __esm = (fn, res) => function __init() {
  return fn && (res = (0, fn[__getOwnPropNames(fn)[0]])(fn = 0)), res;
};
var __copyProps = (to, from, except, desc) => {
  if (from && typeof from === "object" || typeof from === "function") {
    for (let key of __getOwnPropNames(from))
      if (!__hasOwnProp.call(to, key) && key !== except)
        __defProp(to, key, { get: () => from[key], enumerable: !(desc = __getOwnPropDesc(from, key)) || desc.enumerable });
  }
  return to;
};
var __reExport = (target, mod, secondTarget) => (__copyProps(target, mod, "default"), secondTarget && __copyProps(secondTarget, mod, "default"));
var __toCommonJS = (mod) => __copyProps(__defProp({}, "__esModule", { value: true }), mod);
var events_exports = {};
var init_events = __esm({
  "external:events"() {
    __reExport(events_exports, events_star);
  }
});
var EventEmitter = (init_events(), __toCommonJS(events_exports));
var sample_dep_default = class extends EventEmitter {
};
var sample_dep_default2 = sample_dep_default;
function test() {
  return new sample_dep_default2();
}
export {
  test
};

running which will throw the following error

var sample_dep_default = class extends EventEmitter {
                         ^

TypeError: Class extends value #<Object> is not a constructor or null
    at file:///Users/jibyjose/Projects/vite-issue/dist/index.mjs:28:26
    at ModuleJob.run (node:internal/modules/esm/module_job:193:25)
    at async Promise.all (index 0)
    at async ESMLoader.import (node:internal/modules/esm/loader:541:24)
    at async loadESM (node:internal/process/esm_loader:83:5)
    at async handleMainPromise (node:internal/modules/run_main:65:12)

IMHO it happens because, due to the transformations the default export is lost, instead of EventEmitter being the default exported class, it's just an object with all the named exports.

[jiby-aurum]

@patak-dev well I created a better minimal repro example, you can find it here https://github.com/jiby-aurum/vite-issue

It basically re-exports gremlin package, to trigger the optimiser to build gremlin. If you npm i && npx vite build && node dist/index.mjs, you will be able to repro

P.S I do not create an issue, as its experimental now anyways, let me know if I should create one

[patak-dev]

We are gathering feedback to stabilize deps optimization during build time in a future release here:

• Stabilizing deps optimization at build time (and SSR) #13839