feat: optimizeDeps.disabled

[patak-dev]

Description

See vitest-dev/vitest#485 (comment) for context.

Vitest is currently trying to disable deps optimization by using optimizeDeps: { entries: [] }, but this only avoids scanning.

I choose optimizeDeps: { disabled: true } to avoid a breaking change, but if we want to switch to optimizeDeps: OptimizeDepsOptions | false in ResolvedConfig we could do that instead and push this in Vite 3.0.

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

[bluwy]

+1 for optimizeDeps: OptimizeDepsOptions | false. I don't think it would be a breaking change if we go with this though 🤔

[patak-dev]

@bluwy we are going to get TS errors like:

(!) Plugin typescript: @rollup/plugin-typescript TS2339: Property 'exclude' does not exist on type 'false | Omit<DepOptimizationOptions, "keepNames">'.
  Property 'exclude' does not exist on type 'false'.
src/node/optimizer/esbuildDepPlugin.ts: (110:55)

110           if (moduleListContains(config.optimizeDeps?.exclude, id)) {

And it actually makes it harder to use. I also like optimizeDeps: false better, maybe it should be false in the User Config, and then undefined in that case in the Resolved config?

[bluwy]

Good point, it does make it harder to use internally and externally. What about optimizeDeps: OptimizeDepsOptions | null instead? 🤔 I see build.watch uses this signature too.

[patak-dev]

Good point, it does make it harder to use internally and externally. What about optimizeDeps: OptimizeDepsOptions | null instead? 🤔 I see build.watch uses this signature too.

Interesting. It would still be a breaking in the types. As resolvedConfig.optimizeDeps was always defined before. So we have:

(!) Plugin typescript: @rollup/plugin-typescript TS2531: Object is possibly 'null'.
src/node/optimizer/esbuildDepPlugin.ts: (43:28)

43   const allExternalTypes = config.optimizeDeps.extensions
                              ~~~~~~~~~~~~~~~~~~~

src/node/optimizer/esbuildDepPlugin.ts: (45:20)

45         (type) => !config.optimizeDeps.extensions?.includes('.' + type)
                      ~~~~~~~~~~~~~~~~~~~

But at least it is easy to use if we set it null when disabled because config.optimizeDeps?.extensions works (that was the issue we had when using false)

I think this is a better API though. @antfu, what do you think? Maybe we should use optimizeDeps: OptimizeDepsOptions | null as @bluwy is proposing and in the meantime, we could add a proxy that ignores the server._optimizedDeps setter in Vitest (this will have the same effect as disabling optimization)

[antfu]

I think we could introduce both. optimizeDeps: false for user config, and resolve it to optimizeDeps.disabled: true for ResolvedConfig to consume.

[patak-dev]

I think we could introduce both. optimizeDeps: false for user config, and resolve it to optimizeDeps.disabled: true for ResolvedConfig to consume.

Oh, I like it. Almost no one will disable it, so we shouldn't make everybody pay for this feature. Ok, I'm going to change the PR 👍🏼

[bluwy]

Is it safe for type mismatches between the user config and resolved config? I'm also not sure if mergeConfig handles that well. This may also mean the internal code that uses ResolvedConfig have to check .disabled before using .include (for example), instead of ?.include directly.

[patak-dev]

Is it safe for type mismatches between the user config and resolved config? I'm also not sure if mergeConfig handles that well. This may also mean the internal code that uses ResolvedConfig have to check .disabled before using .include (for example), instead of ?.include directly.

We already have a mismatch, as currently User Config is OptimizeDepsOptions | undefined and the resolved config is OptimizeDepsOptions (minus some deprecated option)
And we only have to check for config.optimizeDeps.disabled once when creating the optimizer in the server. And I think the user could also access include, exclude, and other things independently of the optimizer being enabled or disabled, no? So no need to check for every access

[bluwy]

Yeah perhaps that is fine now thinking more about it. It feels weird initially as we're deriving a different resolved config shape based on the user config, but there's resolvedConfig.isProduction too so we already have a pattern like this. We could try out Anthony's method then, and maybe the pattern sticks too.

And I think the user could also access include, exclude, and other things independently of the optimizer being enabled or disabled, no? So no need to check for every access

That's true too. I thought there wouldn't be a reason to access include/exclude if optimizeDeps is turned off, but maybe there are usecase out there where this is needed.

[patak-dev]

Updated, and marked as experimental. @antfu I think we could merge this feature as experimental and undocumented and release it in the next patch as Vitest is going to be the only consumer until Vite 3.0. And we can discuss in a team meeting before the v3 release to accept or modify this API. What do you think?

[patak-dev]

This is causing issues in Marko: https://github.com/vitejs/vite-ecosystem-ci/runs/6001834896?check_suite_focus=true

I'm going to revert the OptimizeDepsOptions | false, and only leave the new experimental disabled