keepNames is broken for _some_ classes

[flunderpero]

I am not able to find the cause or the difference, but some classes are transpiled with an underscore-prefix and others are not. For those starting with an underscore no __name-call is generated and thus .name gives the wrong name.

I am working on a reproducible test, but I cannot reproduce it outside of our (quite large) codebase.

Edit: It seems that working classes are created using the class A ... syntax, where non-working classes are created as const A = ...

[evanw]

It would be very helpful to have a smaller reproducible test case. I'm wondering if this used to work and was broken by #2062. If so, then it should be an issue in version 0.14.26 but not in version 0.14.25. That could be a good quick check to do.

[flunderpero]

As I said, I am trying to reproduce this outside of our code base. I am trying to update from 0.14.18 to 0.14.30 (I should have mentioned that, sorry). And I immediately had #2062 in mind.

[flunderpero]

0.14.25 is working fine. I cannot see why esbuild is renaming some classes in the first place. The class names are unique throughout our code base. But I don't understand the inner workings of esbuild enough, to judge or comment on that really.

[evanw]

Adding the unactionable label until there is actionable information provided here.

I cannot see why esbuild is renaming some classes in the first place. The class names are unique throughout our code base.

This depends on a number of things. During code generation, names are renamed to avoid collisions with identically-named symbols starting at the root scope and propagating inward to child scopes. So colliding symbol names can be top-level symbols in other files, identically-named symbols in higher scopes, or even potentially invisible symbols that esbuild generated during the bundling process but then didn't end up using. We'd have to see a specific example to be able to figure out what's going on.

[struggleRoue]

i have a question, It seems that version 30 causes an error, why rename the after redefined this Modal;

[evanw]

Is this still a problem in version 0.14.31? I had a report of something that wasn't working regarding default-exported classes (#2151). I never got a reproduction case so I don't have test coverage for it, but the reporter said that version 0.14.31 fixed the regression. If you have a reproduction case it would still be useful so I have test coverage to make sure it doesn't regress again in the future.

[struggleRoue]

this problem has been fixed in version 0.14.31, can i know what caused it ?

[evanw]

I believe this was the commit that fixed it: e09acfc. A default-exported class potentially has up to three identifiers: one for the default keyword, one for the mutable optional class name, and one for the immutable binding of the optional class name within the class body. Sometimes the optional class name needs to be automatically generated during the bundling process if it's missing, either to implement syntax lowering of the class body or to transform the default-exported class into a regular class declaration.

The fix was something about reusing the default keyword identifier as the generated class name instead of generating a new identifier. Generating a new identifier should have worked because esbuild supports merging the two symbols together into one symbol later on. I can't say why that fixed it without a way to reproduce the issue. One guess is that esbuild's parser has a bug with tree shaking where merged symbols aren't handled correctly, since symbols are rarely merged in the parser (symbol merging typically happens during linking instead). Or it could be some other kind of bug.

[struggleRoue]

Thank you so much for sharing

[flunderpero]

@evanw I just manage to construct a simple test-case:

# test.ts

export class NotWorking {
    static NAME = <const>"name"

    static create(): NotWorking {
        return new NotWorking()
    }
}

export class Working1 {
    static create(): Working1 {
        return new Working1()
    }
}

export class Working2 {
    static NAME = <const>"name"
}

console.log(NotWorking.name)
console.log(Working1.name)
console.log(Working2.name)

Buliding with esbuild test.ts --format=cjs --keep-names > test.js && node test.js prints:

_NotWorking
Working1
Working2

Buliding with esbuild test.ts --format=cjs --keep-names --minify > test.js && node test.js prints:

NotWorking
Working1
Working2

It seems that keep-names is ignored when minify is not set. The expectation would be that names are not mangled at all if minify is not given, wouldn't it?

In case you are wondering: We don't bundle and don't minify our node code. There is no performance to gain and peeking at the files is much easier.

[evanw]

Thanks very much for the test case. The problem is that #2062 omits name-keeping when the symbol's name hasn't changed under the assumption that the symbol's initializer will still cause the symbol to have that name. It turns out that's an invalid assumption if the symbol's initializer is moved into a temporary variable, as is the case here. I'm just going to revert #2062 to get this working again. The fix for this will go out in the next release.