Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for properties mangling #218

Closed
timocov opened this issue Jul 4, 2020 · 15 comments
Closed

Add support for properties mangling #218

timocov opened this issue Jul 4, 2020 · 15 comments

Comments

@timocov
Copy link

timocov commented Jul 4, 2020

Terser doc: https://terser.org/docs/cli-usage#cli-mangling-property-names-mangle-props

@evanw
Copy link
Owner

evanw commented Jul 5, 2020

Thanks for logging this issue. I'm aware of this transform and I have been considering implementing it. It should be somewhat straightforward I think, except there are some special cases to handle with automatically-generated properties that appear during bundling.

@TravisSpomer
Copy link

TravisSpomer commented Jan 4, 2021

This would be a really helpful improvement! I was kind of surprised that it didn't already happen.

It seems like it could be safely on-by-default while breaking a very small amount of code for:

  • Anything private in TypeScript
  • Private class fields that start with # (I've never heard of anyone actually using that syntax, but it's the closest we have to a standard I guess?)
  • Anything that matches a user-supplied regex (like --mangle-props regex=/^_/ in Terser) for code that isn't using TypeScript

Those defaults would probably get me 100% of what I'd need and want, without breaking anything, and without having to dig any deeper into configuration.

@evanw
Copy link
Owner

evanw commented Jan 4, 2021

Private class fields that start with # (I've never heard of anyone actually using that syntax, but it's the closest we have to a standard I guess?)

This should already happen in esbuild when --minify-identifiers is enabled. Please let me know if you run into any problems with it! I also haven't heard of anyone actually using this syntax either though.

@TravisSpomer
Copy link

Oh, you're totally right. I didn't even realize that I'd updated to a new enough version of TypeScript that doesn't barf on that syntax. My apologies for not retrying that.

Private class fields are indeed getting renamed, to WeakMap.set calls when target is es6 or es2020 or anything else below esnext; and #x on esnext.

I converted a small TypeScript codebase to use # instead of private to explore:

  • Switching my code from private to # in my code to take advantage of the minification actually increased the compressed output size by 6.6% thanks to the WeakMaps.
  • Using # along with target: "esnext" improves things a lot: a reduction of 1.6% versus compressed baseline.
  • If ESBuild had left out the # (yes, slightly changing the meaning of the code) it could have achieved a 3.3% reduction versus compressed baseline.
  • If ESBuild had just mangled all private fields the way it does for # in esnext, it would have been the same 3.3% reduction. (I hand-mangled every private TypeScript field name to test this.)

So my recommendation / request would be:

  • ESBuild should mangle the names of private class fields in TypeScript as there's a clear and safe-seeming 3.3% savings to be had there, and it seems like you have most of the infrastructure in place already.
  • ESBuild should consider mangling names of # class fields and changing them into regular non-private fields, regardless of target, even though that's not technically correct. (You'd know much better than me how likely it would be that that would break existing code. I figure the most likely failure would be anything that enumerates the class's properties, but name mangling would break that regardless...) That would yield an output savings of 1.6% on esnext and 10.3% on any other target.

(I'm aware that those numbers all depend wildly based on the codebase and how long one's field names tend to be. Just trying to be helpful, not demanding! 🙂)

@fabiospampinato
Copy link

Mangling properties leads to a double-digit (~15%) improvement in the minified and gizpped size of the main bundle of the thing I'm working on, it'd be nice to be able to just use esbuild and still gain the same benefits rather than piping esbuild's output to something like terser.

@mindplay-dk
Copy link

In theory, it should be possible get much higher gains with TypeScript code than with JS and terser, since it contains rich type-information available to inform the minifying/mangling process.

See the description of tscc, which tries to implement this. (I haven't tried it myself yet.)

I don't know whether ESBuild internally preserves or discards type-information? If types aren't part of it it's internal representation, then something like tscc probably isn't possible without major changes...

@fabiospampinato
Copy link

@mindplay-dk types just get stripped out in esbuild. I didn't know about tscc, it sounds interesting, but something like that should be officially supported to be made reliable I think, and unfortunately the typescript folks aren't that interested in that microsoft/TypeScript#8

@timocov
Copy link
Author

timocov commented Jun 26, 2021

See the description of tscc, which tries to implement this. (I haven't tried it myself yet.)

Also https://github.com/timocov/ts-transformer-properties-rename

@mindplay-dk
Copy link

Aha, yes, transformers sort of is what I thought plugins were, I guess 😊

@evanw
Copy link
Owner

evanw commented Jan 31, 2022

In theory, it should be possible get much higher gains with TypeScript code than with JS and terser, since it contains rich type-information available to inform the minifying/mangling process.

TypeScript is fundamentally unsound by design, so I believe type information cannot be safely used to make code transformation decisions. Here's an example of the unsoundness: #771 (comment).

@evanw evanw closed this as completed in 47773fc Jan 31, 2022
@evanw
Copy link
Owner

evanw commented Feb 1, 2022

This feature has been implemented and is now documented here: https://esbuild.github.io/api/#mangle-props

@timocov
Copy link
Author

timocov commented Feb 1, 2022

Hey @evanw, thanks for fixing this. Is there any reason why esbuild doesn't handle a key in in operator to mangle? Looks like terser handles this now terser/terser#748. Also, doesn't it look desired to mangle quoted properties and they are match regexp?

image

I do understand that it seems that your behaviour is close to Google Closure Compiler is some way, but this looks like a requirement of Google Closure Compiler to make the code works.

@evanw
Copy link
Owner

evanw commented Feb 2, 2022

Yes, I had Google Closure Compiler's behavior in mind when building this. From Google's documentation:

  • Using string names to refer to object properties:

    The Compiler renames properties in Advanced mode, but it never renames strings.

    var x = { renamed_property: 1 };
    var y = x.renamed_property; // This is OK.
    
    // 'renamed_property' below doesn't exist on x after renaming, so the
    //  following evaluates to false.
    if ( 'renamed_property' in x ) {}; // BAD
    
    // The following also fails:
    x['renamed_property']; // BAD

    If you need to refer to a property with a quoted string, always use a quoted string:

    var x = { 'unrenamed_property': 1 };
    x['unrenamed_property'];  // This is OK.
    if ( 'unrenamed_property' in x ) {};   // This is OK

The rule is "quoted property names are not mangled" and if ('_internal_123123' in a) is a quoted property name, so it won't be mangled. But if (a._internal_123123 !== undefined) isn't quoted so it will be mangled.

@timocov
Copy link
Author

timocov commented Feb 4, 2022

But if (a._internal_123123 !== undefined) isn't quoted so it will be mangled.

@evanw This is not the same as '_internal_123123' in a because it will call a getter for a property rather than in check won't.

The reason why I asked this is that it looks like the following pattern might be quite common for some of users, especially if you're using typescript (but not only):

interface Foo {
    prop: string;
}

interface Baz {
    prop2: number;
}

declare const value: Foo | Baz;

if ('prop' in value) {
    // value is type of Foo here and the compiler understands that
    value.prop;
}

and we cannot use a.b !== undefined in the same way without "casting" to any, unfortunately.

And this pattern looks common for a JavaScript code as well, not just for TypeScript. If you want to use Google Closure Compiler in advanced mode you have to follow its limitations and rules (and this not the only one), but it won't be a JavaScript code, it will be a code which works in Google Closure Compiler (not only there, but especially there), i.e. kind of subset of JavaScript. So you could use esbuild with properties mangling for the code "adapted" for Google Closure Compiler's limitations only, and that's what I wanted to say. But if you're fine with that and you did this intentionally then OK. Probably an option to specify where it should be handled might be a solution though.

evanw added a commit that referenced this issue Mar 3, 2022
@evanw
Copy link
Owner

evanw commented Mar 3, 2022

You make a good case for this. Thanks for the additional information. I'm adding a --mangle-quoted flag to enable this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants