esbuild keepNames not respected

spence-novata commented 1 year ago

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

[X] Follow our Code of Conduct
[X] Read the Contributing Guidelines.
[X] Read the docs.
[X] Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
[X] 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.
[X] Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
[X] The provided reproduction is a minimal reproducible example of the bug.

devjiwonchoi commented 1 year ago

https://github.com/vitejs/vite/pull/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 commented 1 year ago

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 commented 1 year ago

I mean the option itself is disabled in vite:

Code from Vite.js repo

Screenshot 2023-07-06 at 7 55 42 PM

// 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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

sheremet-va commented 1 year ago

This then?

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

spence-novata commented 1 year ago

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

HammCn commented 1 year ago

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 commented 1 year ago

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 commented 8 months ago

is this still a bug?

illegalnumbers commented 8 months ago

i would also like 'keepNames' to be respected

bluwy commented 1 week ago

I dug into this issue today. The reason why we had forced keepNames: false is due to https://github.com/vitejs/vite/issues/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: https://github.com/evanw/esbuild/issues/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 commented 1 week ago

But why do we need it during dev? 🤔

bluwy commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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

vitejs / vite