Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: optimizeDeps.disabled #7646

Merged
merged 3 commits into from Apr 13, 2022
Merged

feat: optimizeDeps.disabled #7646

merged 3 commits into from Apr 13, 2022

Conversation

patak-dev
Copy link
Member

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

@patak-dev patak-dev added the p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) label Apr 8, 2022
@patak-dev patak-dev requested a review from antfu April 8, 2022 06:51
@bluwy
Copy link
Member

bluwy commented Apr 8, 2022

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

@patak-dev
Copy link
Member Author

@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
Copy link
Member

bluwy commented Apr 11, 2022

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
Copy link
Member Author

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
Copy link
Member

antfu commented Apr 11, 2022

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

@patak-dev
Copy link
Member Author

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
Copy link
Member

bluwy commented Apr 11, 2022

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
Copy link
Member Author

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
Copy link
Member

bluwy commented Apr 11, 2022

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
Copy link
Member Author

patak-dev commented Apr 11, 2022

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?

Co-authored-by: Bjorn Lu <bjornlu.dev@gmail.com>
@patak-dev patak-dev merged commit 48e038c into main Apr 13, 2022
@patak-dev patak-dev deleted the feat/optimize-deps-false branch April 13, 2022 04:34
@patak-dev
Copy link
Member Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

4 participants