feat: add optimizeDeps.esbuildOptions

[nihalgonsalves]

Description

This adds the ability to customise the esbuild options during the optimiser phase, including plugins.

There are some libraries that esbuild throws an error on, and I'd really like to have this functionality – we're stuck on a Vite conversion because of one or two packages – with plain esbuild, we're able to use plugins to work around small errors.

Additional context

See #3124

This is inspired by the conversation here: #2886 (comment)

The discussion is about where the build options should live. I feel like since optimizeDeps is an entirely different build phase than the esbuild transform options, the settings should go into config.optimizeDeps.

Questions

• Other than define and entryPoints, should any other options be disallowed? For example, perhaps overriding bundle, format, or outdir shouldn't be allowed.
• Should the old fields be deprecated, or just left as additional options?

---

What is the purpose of this pull request?

• Bug fix
• New Feature
• Documentation update
• Other

Before submitting the PR, please make sure you do the following

• Read the Contributing Guidelines.
• Read the Pull Request Guidelines and follow the Commit Convention.
• Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
• Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. fixes #123).
• Ideally, include relevant tests that fail without this PR but pass with it.

---

Closes #2886

[qnp]

Thanks for your PR, related to mine. In my work I have also to include eslint plugins in this scan phase :

vite/packages/vite/src/node/optimizer/scan.ts

Line 96 in 7bcbe90

build({

1. I'm not 100% sure on whether all passed esbuild options through config.optimizeDeps should be also present here.

2. Regarding your 1st question, I think the Vite core team should clarify us their philosophy. To me, I would keep Vite as flexible as possible, with warnings on what should and what should not be done in the documentation.

3. Regarding your second question, your strategy to deprecate these redundant fields seems right.

[qnp]

Yet, I'm not convinced about the choice to place esbuildOptions into optmimizeDeps. For me, it is not clear to have config.optmimizeDeps in one side with some esbuild build options in it, and config.esbuild in another side with some esbuild transform options. We should work on clarifying this API.

Keeping your idea in mind, we should also deprecate config.esbuild options in favor of:

config = {
  transformDeps: {
    include: ...,
    exclude: ..., // deprecate ??
    jsxInject: `import React from 'react'`,
    esbuildOptions: {
      jsxFactory: 'h',
      jsxFragment: 'Fragment',
      /* any other esbuild _transform_ option */
    }
  }
}

[patak-dev]

@nihalgonsalves we discussed with Evan, this PR is a good first step towards allowing users to patch the optimizer when needed. We can continue talking about other further improvements in #3124, but for the time being, I think that this is enough given the tradeoffs regarding increasing Vite's API surface. Thanks again for pushing for these changes.

@qnp about #2991 (comment), I agree with you that this is a better API for config.esbuild. Let's discuss about this in a separate PR though. It shouldn't be transformDeps as this is applied to your own sources too, maybe just transform. My initial thoughts on this are that we should wait a bit more to change this though.

Regarding #2991 (comment),

I'm not 100% sure on whether all passed esbuild options through config.optimizeDeps should be also present here.

I see two options here. We could add a new optimizeDeps.scan.esbuildOptions, to be discussed in another PR. Or we could review if it makes sense to use some of optimizeDeps.esbuildOptions in

vite/packages/vite/src/node/optimizer/scan.ts

Lines 94 to 105 in 1e785d1

	await Promise.all(
	entries.map((entry) =>
	build({
	entryPoints: [entry],
	bundle: true,
	format: 'esm',
	logLevel: 'error',
	outdir: tempDir,
	plugins: [plugin]
	})
	)
	)

I am not sure at this point what is the best strategy. @nihalgonsalves @antfu could you help to review this?

Regarding your 1st question, I think the Vite core team should clarify us their philosophy. To me, I would keep Vite as flexible as possible, with warnings on what should and what should not be done in the documentation.

At this point, Vite shouldn't tie itself too hard to the internal libraries it is using. For example, IMO it would have been better to name build.watch like build.chokidarOptions because it is easier to deprecate and move to a different lib later. Better than that is having our own API that doesn't depend on the used tools. For example optimizeDeps.exclude, that can be rewired if we ever need to change it.

Regarding your second question, your strategy to deprecate these redundant fields seems right.

I agree here about keepNames since that is more like an esbuild only detail. But optimizeDeps.exclude is a Vite feature. I think we should merge it with optimizeDeps.esbuildOptions.external instead of deprecating it.

[nihalgonsalves]

@patak-js All done.

About the question about dep-scanning: this PR now also uses the same custom options for dep scanning. I think that if you're trying to patch something or change behaviour, you'd want your settings to apply to both the pre-scan and the bundling.

Some explanations of the choices I made below:

[nihalgonsalves]

Added this because I don't believe changing these would have a valid use case. Vite depends on bundling, the metafile, the output settings, and the build mode (i.e. watch = false).

[Shinigami92]

Should we add a comment within the code about why they are omitted?
Or maybe extract them into a well named type 🤔

[nihalgonsalves]

I like the inline documentation of what's omitted, but I added a comment.

e.g.:

[patak-dev]

@nihalgonsalves I tested locally and found several spurious console.log. They are also in the CI https://github.com/vitejs/vite/pull/2991/checks?check_run_id=2483662241#step:11:11.

[nihalgonsalves]

I tested locally and found several spurious console.log. They are also in the CI #2991 (checks).

@patak-js Oops, forgot about those. They're due to the test for the keepNames deprecation handling. I silenced them now

[qnp]

Thanks for the work, LGTM