refactor: remove build time pre-bundling #15184

patak-dev · 2023-11-29T18:25:19Z

Closes #8464
Closes #9171
Closes #9549
Closes #13018
Closes #14106

Description

Starting from #8280, we added experimental support for build time deps optimization. An idea by Evan, that was originally proposed at #4921. The idea was sound, removing a big difference between dev and build (no @rollup/plugin-commonjs, same esbuild pipeline for dependencies in both cases) and hopefully speeding up processing on warm builds (once deps were cached).

In practice, the extra complexity of doing deps optimization at build time and added points of failures made this feature stay experimental until now, seeing little usage. We had issues with rollup not being able to correctly tree-shake dependendencies (that we probably could have solved eventually, but is a good example).

With Rollup 4 switching its parser to native, and Rolldown being worked on, both the performance and the dev-vs-build inconsistency story for this feature are no longer valid. If we want to focus on improving dev/build consistency, then using Rolldown for prebundling during dev and Rolldown as-is during build is a better bet moving forward. Rolldown may also implement caching in a way that is a lot more efficient during build than deps prebundling.

This PR removes the feature, and also removes optimizeDeps.disabled: 'build' | 'dev' | boolean with it. We now have optimizeDeps.noDiscovery. So the optimizer can be disabled by setting optimizeDeps.noDiscovery: true and leaving optimizeDeps.include empty.

The PR keeps SSR deps optimization during dev. In this case, ssr.optimizeDeps.noDiscovery is always true, and the user must manually add deps to optimize to ssr.optimizeDeps.include. When we added this feature, we made noExternal auto-populate ssr.optimizeDeps.include. This PR removes this heuristic, because it means we will need a new config option to disable the SSR dev optimizer and also forced exclude to work differently. Given that noExternal/external may also be reviewed later, it is better to keep include explicit. The user can easily share the array if they would like all noExternal deps to be optimized. @bluwy raised the question if it is justified to keep deps optimization during SSR dev. The complexity is fairly low and it is useful to workaround some issues, so for now, it is kept in this PR. It is still experimental, so we can review and remove it later on too if that is desired.

Related feedback discussions:

What is the purpose of this pull request?

Bug fix
New Feature
Documentation update
Other

stackblitz · 2023-11-29T18:25:22Z

Run & review this pull request in StackBlitz Codeflow.

patak-dev · 2023-11-29T20:11:36Z

I don't know why the docs are failing in netlify, they are building correctly locally.
Edit: it is failing in other PRs too #15187

patak-dev · 2023-11-29T20:14:32Z

playground/ssr-noexternal/__tests__/ssr-noexternal.spec.ts

+test.runIf(!isBuild)('message from require-external-cjs', async () => {
  await page.goto(url)
  expect(await page.textContent('.require-external-cjs')).toMatch('foo')
 })


@sapphi-red I had to skip this test during build. It seems it only works with deps optimization during build.

👍 I think it's fine for now. To make this work, we'll need to convert import foo from 'foo' into import module from 'node:module'; const foo = module.createRequire(import.meta.url)('foo') in this case.

dominikg · 2023-11-29T21:19:47Z

this could break vite-plugin-svelte, which checks optimizeDeps.disabled but doesn't know about optimizeDeps.noDiscovery. If i understand it correctly noDiscovery isn't the same as disabled. noDiscovery prevents the auto-scanner, but user config/plugins like v-p-s would still be able to add to optimizeDeps.include. whereas disabled=true would turn it off completely.

The documentation here https://vitejs.dev/config/dep-optimization-options.html#optimizedeps-disabled only mentions that optimizing in build mode is experimental, not that optimizeDeps.disabled as a flag itself is.

So to not break, i suggest to keep the disabled option, but change it so that false/'dev' log a warning that they are no longer supported. and with 6.0 we can change it to disabled: boolean

dominikg · 2023-11-29T21:20:10Z

/ecosystem-ci run vite-plugin-svelte

vite-ecosystem-ci · 2023-11-29T21:22:41Z

📝 Ran ecosystem CI on 8d6f5ac: Open

suite	result	latest scheduled
vite-plugin-svelte	❌ failure	✅ success

brc-dd · 2023-11-30T01:04:08Z

Edit: fixed in tsx, install tsx@4.6.1

For docs, you can set NODE_VERSION to 20 in netlify.toml. Node v18.19 changed some stuff about loaders that's breaking tsx. Created an issue upstream - privatenumber/tsx#421

patak-dev · 2023-11-30T10:06:34Z

/ecosystem-ci run

patak-dev · 2023-11-30T10:09:11Z

5da3702 brings back optimizeDeps.disabled with compat support and a warning.

The documentation here vitejs.dev/config/dep-optimization-options.html#optimizedeps-disabled only mentions that optimizing in build mode is experimental, not that optimizeDeps.disabled as a flag itself is.

Just for reference, both the docs and the types have optimizeDeps.disabled flag as experimental.

vite-ecosystem-ci · 2023-11-30T10:37:42Z

📝 Ran ecosystem CI on 5da3702: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	❌ failure	❌ failure
ladle	✅ success	✅ success
laravel	❌ failure	❌ failure
marko	✅ success	✅ success
nuxt	❌ failure	❌ failure
nx	✅ success	✅ success
previewjs	✅ success	✅ success
qwik	✅ success	✅ success
rakkas	✅ success	✅ success
sveltekit	❌ failure	❌ failure
unocss	✅ success	✅ success
vike	✅ success	✅ success
vite-plugin-pwa	✅ success	✅ success
vite-plugin-react	✅ success	✅ success
vite-plugin-react-pages	✅ success	✅ success
vite-plugin-react-swc	✅ success	✅ success
vite-plugin-svelte	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vite-setup-catalogue	❌ failure	✅ success
vitepress	✅ success	✅ success
vitest	✅ success	✅ success

sapphi-red · 2023-11-30T12:06:20Z

/ecosystem-ci run vite-setup-catalogue

vite-ecosystem-ci · 2023-11-30T12:09:55Z

📝 Ran ecosystem CI on 5da3702: Open

suite	result	latest scheduled
vite-setup-catalogue	✅ success	✅ success

ChuHoMan · 2023-12-01T03:42:13Z

playground/optimize-deps/__tests__/optimize-deps.spec.ts

@@ -141,7 +141,7 @@ test('vue + vuex', async () => {

 // When we use the Rollup CommonJS plugin instead of esbuild prebundling,
 // the esbuild plugins won't apply to dependencies
-test('esbuild-plugin', async () => {
+test.runIf(isServe)('esbuild-plugin', async () => {


I really appreciate your work, but I have some feedback I'd like to share. There are some custom esbuild plugins in my app to handle special files in dependencies, as described in docs. If the pre-build feature is removed at build time, is there any other way to keep the custom esbuild plugin consistent in build mode?

You'll need to have the same custom handling implemented as a rollup plugin. It would be good if you link to the way you are using it for reference, but I think it would be hard to justify the complexity this PR is removing as it can be still make to work. And later on if we move to Rolldown instead of esbuild, then you will get the consistency back for dependency handling.

sapphi-red · 2023-12-02T16:18:15Z

Even without disabled: true, I think adding a plugin that executes config.optimizeDeps.include = [] in configResolved hook will work. (it can be overridden by subsequent plugins but that same with optimizeDeps.disabled)

If we keep disabled, I vote for your idea of adding enabled. I think we should have a warning for optimizeDeps.disabled: false.

dominikg · 2023-12-02T17:10:55Z

indifferent whether if should be called disabled or enabled. Why would a warning for disabled: false be a problem though? you'd only warn in build mode. and someone who uses disabled: false can remove it to get rid of the warning

patak-dev · 2023-12-02T21:10:22Z

We have these values to take into account:

disabled: 'dev' only enabled during build, I don't think people used this one at all. And we can add a warning without problems.
disabled: true was used to disable the optimizer. No problem at all here.
disabled: 'build' was the default (only enabled during dev). No problem either, but we should change the default to false if we want to keep the flag.
disabled: false enabled both dev and build. If we want to preserve disabled as a flag, we can't warn when this one is used or ask the user to remove it. This should be a valid value moving forward. Even if we could, we can't also set false as the default or we won't know if we should warn or not (because of plugins being able to set it in the config hook)

If we keep disabled, we can't properly warn about the commonjs plugin being disabled either. As there may be valid reasons for that.
If you see a good way to add the warning still (without asking the user to remove disabled: false), let me know, maybe I missed something when I tried to implement it.

Using include: [], noDiscovery: true in a post plugin, could be an option but I'm leaning now to have an easier way to disable the optimizer, at least for debugging. I think we should add enabled: boolean and properly deprecate the disabled flag.

bluwy · 2023-12-06T12:46:42Z

I think I'm leaning towards keeping disabled. Maybe we can check specifically that disabled: false is set from the user config specifically (before plugins alter them), and that commonjsOptions.include: [] is also set, then we emit a warning? I think that would be clear enough of the intent. And worst case they could always suppress it manually.

But anyways their build setup may likely fail anyways so we couldn't emit a warning + do something to fix it.

Re enabled, I reminds me that I used to prefer default-false options, hence noDiscovery. There's also the PR that adds holdUntilCrawlEnd. I think past me was over-paranoid and it's not really an issue in retrospect. I'm not sure how others feel about it now but thought to bring this up.

patak-dev · 2023-12-06T13:56:54Z

I think I'm leaning towards keeping disabled. Maybe we can check specifically that disabled: false is set from the user config specifically (before plugins alter them), and that commonjsOptions.include: [] is also set, then we emit a warning? I think that would be clear enough of the intent. And worst case they could always suppress it manually.

I think if we keep disabled, we should allow users to set it to false without a warning. And in that case, commonjsOptions.include: [] could be also configured without relation to the optimizer in some projects.

If we keep it, we could have a transition period, in 5.1 we emit the warning for false asking users to remove it, warn about commonjsOptions.include: [], and automatically enable it. Then on 5.x we stop emitting the warning and disabled: false will be a proper option again. But feels quite complex and won't cover people jumping from <=5.0 directly to 5.x.

Given the low usage of build time optimization, maybe it is better to let projects fail and don't warn if we want to keep the disabled flag?

Re enabled, I reminds me that I used to prefer default-false options, hence noDiscovery. There's also the PR that adds holdUntilCrawlEnd. I think past me was over-paranoid and it's not really an issue in retrospect. I'm not sure how others feel about it now but thought to bring this up.

Ya, me too. But I think it is ok if the resolved config properly sets the value and undefined
isn't allowed there. We have other options like server.fs.strict for example that are true by default.

bluwy · 2023-12-06T14:30:32Z

I think if we keep disabled, we should allow users to set it to false without a warning. And in that case, commonjsOptions.include: [] could be also configured without relation to the optimizer in some projects.

I don't quite understand this part. We should allow false without a warning. But we could technically detect if it's a false that's explicitly set by the user or not too. If it is, and commonjsOptions.include: [] is set together, I think that's a strong signal that they're relying on the removed build prebundling feature.

Personally I'm also fine with no warning at all too. The build could fail, and they could check the changelog for what caused it to fail. I guess the only concern is that if it's hard to figure out what failed after they upgraded all deps, but they know the risk of using experimental features 😄

patak-dev · 2023-12-06T15:04:00Z

I think if we keep disabled, we should allow users to set it to false without a warning. And in that case, commonjsOptions.include: [] could be also configured without relation to the optimizer in some projects.

I don't quite understand this part. We should allow false without a warning. But we could technically detect if it's a false that's explicitly set by the user or not too. If it is, and commonjsOptions.include: [] is set together, I think that's a strong signal that they're relying on the removed build prebundling feature.

We can detect this, but if we want disabled to be out of experimental at one point, we need the user to be able to set it to false and also be able to use commonjsOptions.include: [] if they want. So if we add a warning, we need that warning to be there for a transition period. At one point, false should be fair play, no matter what other config options are set. I'm ok doing that, and removing the warning in 5.2 or 5.3

bluwy · 2023-12-06T16:31:26Z

Yeah I'm also fine doing that and then removing it in a minor. I was thinking the chances of false positive would be low if we did the warning.

But stabilizing it without the gotchas is a good point. We could both stabilize and remove the warning together in a future minor to justify it.

patak-dev · 2024-01-10T12:32:19Z

@dominikg @bluwy in the previous team meeting, we discussed this PR and decided that if we need a way to disable the optimizer, we better go with a clean new flag enabled, and cleanly deprecate and later remove disabled.

Given that enabled: false is equivalent to noDiscovery: false + include: [], some were still not convinced that the new flag is really needed though. I'm thinking we could merge this PR deprecating disabled, and later on do another feature PR with enabled if we can properly justify that addition. What do you think?

dominikg · 2024-01-10T20:50:14Z

as mentioned earlier disabled/enabled and noDiscovery are different features really.

That being said maybe a full disable is a rare usecase and trimming the option helps keeping the option count manageable? I'm still slightly in favor of keeping it, but wont object removing

patak-dev · 2024-01-11T07:55:15Z

Ok, I think we should deprecate it for now and we can gather more feedback from people as they will see the notice and hopefully will reach out to us about it.

bluwy · 2024-01-11T16:01:57Z

If it's decided to deprecate disabled, I can get by it 😄 And if this means there won't be a stable way to disable the optimizer, I think that's fine too. One could always write a Vite plugin that resets optimizeDeps.include to [] to disable it.

Before merging though, it would be nice to address #15184 (comment), and potentially the other conversations above if they're still relevant.

patak-dev · 2024-01-15T10:35:38Z

/ecosystem-ci run

vite-ecosystem-ci · 2024-01-15T10:52:44Z

📝 Ran ecosystem CI on 2cf8b6e: Open

suite	result	latest scheduled
analogjs	✅ success	✅ success
astro	✅ success	✅ success
histoire	❌ failure	❌ failure
ladle	✅ success	✅ success
laravel	❌ failure	❌ failure
marko	✅ success	✅ success
nuxt	❌ failure	✅ success
nx	✅ success	✅ success
previewjs	✅ success	✅ success
qwik	✅ success	✅ success
rakkas	✅ success	✅ success
sveltekit	✅ success	✅ success
unocss	❌ failure	❌ failure
vike	❌ failure	❌ failure
vite-plugin-pwa	✅ success	✅ success
vite-plugin-react	✅ success	✅ success
vite-plugin-react-pages	✅ success	✅ success
vite-plugin-react-swc	✅ success	✅ success
vite-plugin-svelte	✅ success	✅ success
vite-plugin-vue	✅ success	✅ success
vite-setup-catalogue	✅ success	✅ success
vitepress	❌ failure	❌ failure
vitest	✅ success	❌ failure

XiSenao · 2024-01-27T09:10:31Z

The current PR has removed the pre-bundling behavior from the build phase, so why do we still need to retain the following logic in the build phase?

vite/packages/vite/src/node/build.ts

Lines 527 to 529 in 3fceb14

    
           if (isDepsOptimizerEnabled(config, ssr)) { 
        
             await initDepsOptimizer(config) 
        
           }

This will make the build phase execute pre-bundling actions by default, what does this mean? I'm not sure if there is any other reason.

patak-dev · 2024-01-27T09:33:08Z

Thanks for spotting this @XiSenao, added you as co-author in the PR to remove it #15727

patak-dev added 4 commits November 29, 2023 18:21

refactor: remove build time pre-bundling

e50cd8e

docs: remove deps pre-bundling

a9c5a01

test: update playgrounds

2fd6b1a

test: skip ssr-noexternal test during build

01e2a65

patak-dev added p2-nice-to-have Not breaking anything but nice to have (priority) feat: deps optimizer Esbuild Dependencies Optimization feat: build labels Nov 29, 2023

patak-dev added 2 commits November 29, 2023 19:37

test: remove disabled config

14c963f

chore: remove unneded worker guard to get the optimizer

79f9b14

patak-dev commented Nov 29, 2023

View reviewed changes

chore: remove unneeded workerSources set

8d6f5ac

sapphi-red mentioned this pull request Nov 30, 2023

chore(deps): update all non-major dependencies #15145

Merged

1 task

patak-dev added 2 commits November 30, 2023 08:44

chore: merge main

ad415f2

feat: keep optimizeDeps.disabled as deprecated and emit warning

5da3702

This comment was marked as outdated.

Sign in to view

patak-dev added this to the 5.1 milestone Nov 30, 2023

patak-dev mentioned this pull request Nov 30, 2023

docs: update social links vitest-dev/vitest#4624

Merged

6 tasks

ChuHoMan reviewed Dec 1, 2023

View reviewed changes

chore: merge main

2313e53

patak-dev added 2 commits January 11, 2024 17:05

chore: merge main

3e3f7ef

fix: ssrOptimizeCheck

08cb922

patak-dev mentioned this pull request Jan 12, 2024

import star works in dev but not in prod #15542

Closed

7 tasks

chore: merge main

2cf8b6e

bluwy approved these changes Jan 15, 2024

View reviewed changes

patak-dev merged commit 757844f into main Jan 15, 2024
10 checks passed

patak-dev deleted the refactor/remove-experimental-build-prebundling branch January 15, 2024 13:29

patak-dev mentioned this pull request Jan 27, 2024

fix: do not init optimizer during build #15727

Merged

4 tasks

sapphi-red mentioned this pull request Feb 8, 2024

refactor: remove vite build --force #15837

Merged

9 tasks

hi-ogawa mentioned this pull request Feb 16, 2024

fix(vitest): fix optimizeDeps.disabled warnings on Vite 5.1 vitest-dev/vitest#5215

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: remove build time pre-bundling #15184

refactor: remove build time pre-bundling #15184

patak-dev commented Nov 29, 2023

stackblitz bot commented Nov 29, 2023

patak-dev commented Nov 29, 2023 •

edited

patak-dev Nov 29, 2023

sapphi-red Nov 30, 2023

dominikg commented Nov 29, 2023

dominikg commented Nov 29, 2023

vite-ecosystem-ci bot commented Nov 29, 2023

brc-dd commented Nov 30, 2023 •

edited

patak-dev commented Nov 30, 2023

patak-dev commented Nov 30, 2023

vite-ecosystem-ci bot commented Nov 30, 2023

This comment was marked as outdated.

This comment was marked as outdated.

sapphi-red commented Nov 30, 2023

vite-ecosystem-ci bot commented Nov 30, 2023

ChuHoMan Dec 1, 2023

patak-dev Dec 1, 2023

sapphi-red commented Dec 2, 2023

dominikg commented Dec 2, 2023

patak-dev commented Dec 2, 2023

bluwy commented Dec 6, 2023

patak-dev commented Dec 6, 2023

bluwy commented Dec 6, 2023

patak-dev commented Dec 6, 2023

bluwy commented Dec 6, 2023

patak-dev commented Jan 10, 2024

dominikg commented Jan 10, 2024

patak-dev commented Jan 11, 2024

bluwy commented Jan 11, 2024

patak-dev commented Jan 15, 2024

vite-ecosystem-ci bot commented Jan 15, 2024

XiSenao commented Jan 27, 2024

patak-dev commented Jan 27, 2024

refactor: remove build time pre-bundling #15184

refactor: remove build time pre-bundling #15184

Conversation

patak-dev commented Nov 29, 2023

Description

What is the purpose of this pull request?

stackblitz bot commented Nov 29, 2023

patak-dev commented Nov 29, 2023 • edited

patak-dev Nov 29, 2023

Choose a reason for hiding this comment

sapphi-red Nov 30, 2023

Choose a reason for hiding this comment

dominikg commented Nov 29, 2023

dominikg commented Nov 29, 2023

vite-ecosystem-ci bot commented Nov 29, 2023

brc-dd commented Nov 30, 2023 • edited

patak-dev commented Nov 30, 2023

patak-dev commented Nov 30, 2023

vite-ecosystem-ci bot commented Nov 30, 2023

This comment was marked as outdated.

This comment was marked as outdated.

sapphi-red commented Nov 30, 2023

vite-ecosystem-ci bot commented Nov 30, 2023

ChuHoMan Dec 1, 2023

Choose a reason for hiding this comment

patak-dev Dec 1, 2023

Choose a reason for hiding this comment

sapphi-red commented Dec 2, 2023

dominikg commented Dec 2, 2023

patak-dev commented Dec 2, 2023

bluwy commented Dec 6, 2023

patak-dev commented Dec 6, 2023

bluwy commented Dec 6, 2023

patak-dev commented Dec 6, 2023

bluwy commented Dec 6, 2023

patak-dev commented Jan 10, 2024

dominikg commented Jan 10, 2024

patak-dev commented Jan 11, 2024

bluwy commented Jan 11, 2024

patak-dev commented Jan 15, 2024

vite-ecosystem-ci bot commented Jan 15, 2024

XiSenao commented Jan 27, 2024

patak-dev commented Jan 27, 2024

patak-dev commented Nov 29, 2023 •

edited

brc-dd commented Nov 30, 2023 •

edited