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: optimize deps option to turn off auto discovery #13000

Merged
merged 2 commits into from
Apr 26, 2023

Conversation

patak-dev
Copy link
Member

@patak-dev patak-dev commented Apr 25, 2023

Description

Edit: I ended up renaming it to noDiscovery.

Adds a new experimental boolean option optimizeDeps.noDiscovery, defaulting to true

  /*
   * Automatic dependency discovery. When `noDiscovery` is true, only dependencies 
   * listed in `include` will be optimized. The scanner isn't run for cold start 
   * in this case. CJS-only dependencies must be present in `include` during dev.
   * @default false
   * @experimental
   */
  noDiscovery?: boolean

@sheremet-va is exploring replacing Vitest deps.inline feature with Vite's deps optimization. To make this work, they need auto-discovery of dependencies to be turned off. They would like to have full control of what dependencies get optimized using optimizeDeps.include. We should also merge #12414 to add glob support to optimizeDeps.include

Some other options we evaluated in this gist.

Edit: scratch the next paragraph, as it simplified the logic to rename at the end.
@bluwy also proposed we use noDiscovery instead of autoDiscovery so we can have the option be false by default. I think this is a good idea in general but in this case, I find it a bit more confusing. @bluwy I checked and we have a lot of boolean options that default to true. preTransformRequests for example. Let's see what others think.

It is experimental because @sheremet-va will need to play with this and we may end up changing this feature later. I think we could move forward so they can check what we are missing.


What is the purpose of this pull request?

  • Bug fix
  • New Feature
  • Documentation update
  • Other

@stackblitz
Copy link

stackblitz bot commented Apr 25, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@patak-dev patak-dev added feat: deps optimizer Esbuild Dependencies Optimization p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority) labels Apr 25, 2023
@antfu
Copy link
Member

antfu commented Apr 25, 2023

LGTM! 👍

I wouldn't worry about the default true/false much, as long as we provide jsdocs and documentation. I understand that making options opt-in would be a good design pattern for understanding, but I feel that only work if all the options are falsy default (that might need to whole set of breaking changes)

@patak-dev
Copy link
Member Author

I think the same @antfu. In the end, I switched this one to noDiscovery because if not we would be forced to have new types for resolved Optimize Deps Options, and the logic when using the option was negated all the time.

@patak-dev patak-dev changed the title feat: optimize deps auto discovery feat: optimize deps option to turn off auto discovery Apr 25, 2023
Copy link
Member

@sheremet-va sheremet-va left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this 👍🏻 Thank you 🙏🏻

Vitest probably cannot remove inline for some time now, but if this feature will be more performant then we can recommend it over inline 👍🏻

Copy link
Member

@bluwy bluwy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice tests!

@patak-dev
Copy link
Member Author

Starting discussion to stabilize this feature:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat: deps optimizer Esbuild Dependencies Optimization p3-downstream-blocker Blocking the downstream ecosystem to work properly (priority)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants