esbuild keepNames not respected

[spence-novata]

Describe the bug

Keeps names is being overridden https://github.com/vitejs/vite/blame/6f1c55e7d694dd26f9eca30239faeb5a59c41486/packages/vite/src/node/plugins/esbuild.ts#L226

This is confusing because the type definition for vite.config.ts accepts it.

And whilst generally it doesn't seem to be a problem when a class references itself:

export class MyClass {
  static self() {
    return MyClass
  }
}

esbuild prefixes the class with an underscore.

Originally reported on Vitest, @sheremet-va suggested this was introduced as a fix for #9164

Reproduction

https://github.com/spence-novata/vitest-keep-name-poc

Steps to reproduce

npm i
npm run test

System Info

System:
    OS: macOS 13.4.1
    CPU: (10) arm64 Apple M1 Max
    Memory: 3.83 GB / 64.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 18.3.0 - ~/.asdf/installs/nodejs/18.3.0/bin/node
    npm: 8.11.0 - ~/.asdf/plugins/nodejs/shims/npm
    pnpm: 7.16.0 - /opt/homebrew/bin/pnpm
    Watchman: 2023.03.13.00 - /opt/homebrew/bin/watchman
  Browsers:
    Brave Browser: 113.1.51.118
    Chrome: 114.0.5735.198
    Safari: 16.5.1

Used Package Manager

npm

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to vuejs/core instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[devjiwonchoi]

#9166

Seems like keepNames is not allowed in esbuild.

import { defineConfig } from "vitest/config";

export default defineConfig({
  esbuild: {
    keepNames: true, // ---> not allowed in `esbuild`
  },
  test: {
    globals: true,
  },
})

[spence-novata]

That doesn't seem to be the case for me and looking at the source types:

UserConfig.esbuild is typed as ESBuildOptions
which extends TransformOptions from esbuild
which extends CommonOptions from esbuild
which types its keepNames property as ?: boolean

The main problem is that Vite doesn't expose keepNames. The typing just confuses the issue.

[devjiwonchoi]

I mean the option itself is disabled in vite:

Code from Vite.js repo

// packages/vite/src/node/plugins/esbuild.ts

const transformOptions: TransformOptions = {
    target: 'esnext',
    charset: 'utf8',
    ...esbuildTransformOptions,
    minify: false,
    minifyIdentifiers: false,
    minifySyntax: false,
    minifyWhitespace: false,
    treeShaking: false,
    // keepNames is not needed when minify is disabled.
    // Also transforming multiple times with keepNames enabled breaks
    // tree-shaking. (#9164)
    keepNames: false,
  }

[spence-novata]

Ah oh see, sorry I thought you meant that it's not allowed under with typing.

It looks like whilst generally keepNames is not needed when minify is disabled, if the class references itself it is needed.

[sheremet-va]

I would assume that we need to disable keepNames only when building because the minification is not performed in serve mode:

configResolved(config) {
  if (config.command === 'build') {
    transformOptions.keepNames = false
  }
}

[devjiwonchoi]

I would assume that we need to disable keepNames only when building because the minification is not performed in serve mode:

@sheremet-va Do you mind if I open a PR for that modification?

[spence-novata]

Wouldn't that just fix serve mode and break it when built?

[sheremet-va]

This then?

configResolved(config) {
  if (config.command === 'build' && config.build.minify === 'esbuild') {
    transformOptions.keepNames = false
  }
}

[spence-novata]

Would that then require to minify to be set to terser (or disabled) for keepNames to be respected?

[HammCn]

Same issues in my project

vite: v4.4.9
terser: 5.19.2

only part of the class name and function name kept when i set minify to 'terser'.

but some class name kept in other position:

[TrulyBright]

So the keepNames option disabled in transform phase somehow lets names change.

I think #9166 may as well be reverted unless we get to find a solution that makes both tree-shaking and keepNames work properly. Tree-shaking is about file size while keepNames is about real logic that matters. I'm referencing instance.constructor.name a lot in my project and have no idea how to solve this issue.

[illegalnumbers]

is this still a bug?

[illegalnumbers]

i would also like 'keepNames' to be respected

[bluwy]

I dug into this issue today. The reason why we had forced keepNames: false is due to #9164. If we reverted that, everyone's bundle size is likely to explode because treeshaking won't be working effectively (if they set keepNames: true).

I investigated the source of that issue and filed a feature request to esbuild that should fix it instead: evanw/esbuild#3965. If that's resolved, we should be safe to revert the option.

(Note that if those are resolved, the source issue repro will generate a JS bundle rather than 0 JS, but it's less impactful in practice because treeshaking is still effective, just that it's not smart enough to reduce to 0 JS.)

[sheremet-va]

But why do we need it during dev? 🤔

[bluwy]

I guess we could allow it during dev, but build wouldn't be able to match it if we still set keepNames: false in build and would cause inconsistencies.

[sheremet-va]

I guess we could allow it during dev, but build wouldn't be able to match it if we still set keepNames: false in build and would cause inconsistencies.

True, dev won’t catch the error and it will be hard to debug when it fails in production 😔

[brokenmass]

to add to this: there a lot of scenario that requires class names to be maintained (like if you are using decorators that tracks the name of the class), that gets 'mysteriously' broken by this behaviour.

forcing keepNames: false behind the scene is not transparent (users might find this only in prod when they minify the code) and the fact that it cannot be overridden leaves the user no choice but to use another minifier (terser).

I appreciate your concern about bundle size but to address that you are sacrificing build correctness and that does not sound right