Class renaming breaks certain patterns

[brianmhunt]

It's not uncommon for code to use the class name constructor for certain patterns i.e.

const registry: Record<string, WebComponent> = {}

class WebComponent {
  static register (registrationID = this.constructor.name) { register[registryID] = this }
  static load (registryID: string) { return register[registryID] }
}

class ComponentA extends WebComponent {}

ComponentA.register()

elsewhere

const ComponentA = '123'
WebComponent.load('ComponentA') // fails b/c it's "ComponentA2"

When the esbuild bundler hits a conflicting class name it appends a number, which breaks the pattern.

There are several potential ways to fix this, including (purely for illustration):

1. an optional check (e.g. --no-class-rename) that throws an error rather than adding a number to the class name. (For obvious reasons, this --no-class-rename would also ideally prevent class name mangling during minification).
2. giving priority to class names not being renamed (i.e. maintaining its original name) over other symbols when there's a collision
3. including some source-code comment that specifies that the symbol not be renamed

This is blocking our project from using esbuild, and would also break one of the patterns in the TKO project that I maintain (a successor to/built on knockout.js).

[brianmhunt]

Just having a look, it seems like the solution would be somewhere in the linker or renamer.

Perhaps it's possible (to my naïve eyes) for class names to (optionally) be considered reserved names via ComputeReservedNames.

This looks like it could be done by looking at the symbol kind, or alternatively adding MustNotBeRenamed to the parseClassStmt.

I'm sure this is probably pretty obvious (and quite possibly obviously incorrect) if you're familiar with it, but since esbuild is an important item for us I thought I'd have a look and point in the general direction.

[evanw]

Forcing the name to not change may end up corrupting the build because two symbols from different files can have the same name, and these names will collide and either cause a syntax error or silently shadow the previous name if they end up being hoisted into the same scope. So this change is more complicated than just keeping the names the same.

I think this feature could be made to behave similar to direct eval() which must preserve all symbol names because the code may try to access them. This led me to discover a bug with how direct eval() handles top-level symbols and name collisions. Direct eval() should cause the bundler to treat each file as CommonJS so that each file has its own scope to avoid name collisions. This disables tree shaking since esbuild doesn't do tree shaking for CommonJS modules, but it's the most correct way to do this.

[brianmhunt]

The eval pattern seems like it would work, though it would be a shame to lose tree-shaking. Are you thinking it would be a total loss of tree-shaking, or constrained only to classes (or files with classes)?

On the collision problem, perhaps the reserved name a.) occur only with class name declarations; and b.) raised an error on collision. By illustration, if the following are bundled.

a.ts

class Hello {} // becomes a reserved name

b.ts

const Hello // renamed to Hello2

c.ts

class Hello {} // throws error: duplicate reserved class name

Does this pattern address the hoisting collision problem? The problem is limited to two class declarations of he same name, and in that case an error could be thrown for this.

[evanw]

Actually I think the easiest way to solve this would be to have an option that injects code which assigns to the name property for classes and functions. Something like this:

// original.js
class Hello {}
function hello() {}

// generated.js
let __name = (obj, value) => Object.defineProperty(obj, 'name', {value, configurable: true});
class Hello2 {}
__name(Hello2, 'Hello');
function hello2() {}
__name(hello2, 'hello');

That wouldn't disable tree shaking and would let these symbols still be minified.

[brianmhunt]

I was just thinking in that direction, too. That certainly feels like the least-invasive option. I dare say, "correct".

[brianmhunt]

... the only caveat I have is that this has been a known problem of minification / mangling for a very long time, so I wonder why nobody else had done it. My only guess is that it will not work with IE.

[brianmhunt]

@ctcarton tagging you just in case you have any thoughts on what @evanw has proposed to remedy the class name mangling i.e. Object.defineProperty(obj, 'name', ...), above.

[bathos]

What makes the late-rename necessary? Not sure about TS, but in JS, let Hello2 = class Hello {}; and class Hello2 {} have semantic equivalence apart from what value ends up set as the function's name.

[evanw]

What makes the late-rename necessary? Not sure about TS, but in JS, let Hello2 = class Hello {}; and class Hello2 {} have semantic equivalence apart from what value ends up set as the function's name.

Here's the case I'm worried about. Imagine two files:

// a.js
import {Hello as Foo} from './b'
export class Hello { foo = new Foo }

// b.js
import {Hello as Bar} from './a'
export class Hello { bar = new Bar }

It would be bad if esbuild's scope hoisting pass compiled that to something like this:

// a.js
let Hello = class Hello { foo = new Hello2 }

// b.js
let Hello2 = class Hello { bar = new Hello }

The second class stored in the variable Hello2 would accidentally set bar to an instance of the class from file b.js instead of the class from file a.js because the use of a class expression introduces a new symbol Hello that shadows the top-level symbol Hello. It might be possible to get this to work with enough renaming magic but it seems more complicated.

Another issue is that people are going to want this to work with functions too, and there the same approach doesn't apply because function declarations are hoisted while variable declarations and function expressions aren't. Hoisting can be mitigated to some extent by reordering statements but doing that across files starts to get complex.

Basically assigning to name is the simplest solution.

[bathos]

Makes sense. I can see now how it would end up adding more complexity / special cases to consider — thanks for explaining.

[brianmhunt]

@evanw Just to confirm, the Object.defineProperty proposal looks good.

[brianmhunt]

Fantastic, thanks @evanw !

[bathos]

I can open a different issue for this if it makes more sense, but I just hit the following problem (reproduced here more minimally) and it seems related to what’s been discussed above:

// In:
class Foo {
  static Bar = Foo;
}

// Out:
var Foo = class {
  static Bar = Foo;
};

The output isn’t equivalent and will throw a ReferenceError that the input doesn’t.

[evanw]

Yeah sorry about that. I discovered this myself yesterday. It's already fixed and is going out with the next release: https://github.com/evanw/esbuild/blob/2001dff446c016612f4043c8ff8b7cb1bf69ed5e/CHANGELOG.md#unreleased. It's actually not related to this issue. It's related to issue #478 instead.

[bathos]

Awesome! And wow, that’s some issue ... it’s like tooling dev creepypasta.

[davetron5000]

Anyone coming here from Google, the option --keep-names has been added since this was closed I think. It seems to have solved the problem I had that led me here.