fix: disable keepNames in vite:esbuild (fixes #9164)

[sapphi-red]

Description

Transforming a function by esbuild with keepNames enabled multiple times will break tree-shaking.

Example:

// Input
function a() {}

// After `--tree-shaking`
/* empty */

// After `--keep-names`
var __defProp = Object.defineProperty;
var __name = (target, value) => __defProp(target, "name", { value, configurable: true });
function a() {
}
__name(a, "a");

// After `--keep-names` => `--tree-shaking --minify`
var __defProp=Object.defineProperty,__name=(e,r)=>__defProp(e,"name",{value:r,configurable:!0});function a(){}__name(a,"a");

This PR disables keepNames in vite:esbuild because keepNames is only needed when minify is enabled.

However, minification renames symbols to reduce code size and bundling sometimes need to rename symbols to avoid collisions. That changes value of the name property for many of these cases.
https://esbuild.github.io/api/#keep-names

fixes #9164

Additional context

---

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.

[netlify]

✅ Deploy Preview for vite-docs-main canceled.

Name	Link
🔨 Latest commit	2f48e59
🔍 Latest deploy log	https://app.netlify.com/sites/vite-docs-main/deploys/62d3e1122646070008df579e

[patak-dev]

Great find. For deps optimization, the user should never set keep names then as we don't minify, no? I'm a bit confused as to why keepNames was every added as an option to optimizeDeps in the first place, and I wonder if we should remove the last line from this section as it may be pushing users in the wrong direction: https://vitejs.dev/guide/migration.html#config-options-changes

[sapphi-red]

For deps optimization, the user should never set keep names then as we don't minify, no?

I think we still need it because, bundle: true is used for optimization.

[patak-dev]

Would you explain a bit more why keepNames may be useful in when using bundle: true in optimizeDeps? And if the transformation that is done is similar to the one you shown for minify, wouldn't we have tree-shacking issues for deps because esbuild is executed twice with keepNames on them?

[sapphi-red]

However, minification renames symbols to reduce code size and bundling sometimes need to rename symbols to avoid collisions. That changes value of the name property for many of these cases.
https://esbuild.github.io/api/#keep-names

esbuild says it renames symbols to avoid collisions.

wouldn't we have tree-shacking issues for deps because esbuild is executed twice with keepNames on them?

That's a good point. Yeah I think so.
It seems adding treeShaking=true will prevent the issue. (without tree-shaking=true / with tree-shaking=true)

BTW maybe we don't have treeShaking=true for build dep-optimization?

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

Lines 585 to 617 in e6f3b02

	const result = await build({
	absWorkingDir: process.cwd(),
	entryPoints: Object.keys(flatIdDeps),
	bundle: true,
	// We can't use platform 'neutral', as esbuild has custom handling
	// when the platform is 'node' or 'browser' that can't be emulated
	// by using mainFields and conditions
	platform,
	define,
	format: 'esm',
	// See https://github.com/evanw/esbuild/issues/1921#issuecomment-1152991694
	banner:
	platform === 'node'
	? {
	js: `import { createRequire } from 'module';const require = createRequire(import.meta.url);`
	}
	: undefined,
	target: isBuild ? config.build.target || undefined : ESBUILD_MODULES_TARGET,
	external,
	logLevel: 'error',
	splitting: true,
	sourcemap: true,
	outdir: processingCacheDir,
	ignoreAnnotations: !isBuild,
	metafile: true,
	plugins,
	...esbuildOptions,
	supported: {
	'dynamic-import': true,
	'import-meta': true,
	...esbuildOptions.supported
	}
	})

[patak-dev]

We don't

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

Line 585 in e6f3b02

const result = await build({

But I think this shouldn't affect the result because it is applied to library code that if the library is well packaged it should be already tree shaked. Or maybe I'm missing something here. I would imagine that if the keepNames is applied to an internal non-treeshakeable branch then we still have the issue after the post-minify.

[sapphi-red]

Ah, yes. It will be a problem if a code is not tree-shakable in optimize phase and is tree-shakable in minify phase. For example, when define creates a constant condition.
I'm not sure how to solve this.