Computed or Invalid property names

[artfiedler]

Bug report

Input code

export class testProperties {
    public thank: number = 0;
    public you: number = 0;
    public painInTheButton0: number = 0;
    public painInTheButton1: number = 0;
    public painInTheButton2: number = 0;
}

var test = new testProperties();
test.thank = 0;
test.you = 0;
for (var i = 0; i < 3; i++)
    test['painInTheButton' + i] = i;
var totalThanks = test.thank + 1;
var totalForYou = test['you'] + 0;
var button1Result = test.painInTheButton1;
var button2Result = test['painInTheButton' + 2];
var button3Result = test['painInTheButton3'];

Expected output

error: testProperties.ts:13 Computed property, use /** @ignore-rename:[painInTheButton0,painInTheButton1,painInTheButton2] @ignore-computed-property */
error: testProperties.ts:18 Invalid property, use /** @ignore-invalid-property */

Actual output

(function(exports){
exports.testProperties = void 0;
    var testProperties = (function () {
        function testProperties() {
            this._internal_thank = 0;
            this._internal_you = 0;
            this._internal_painInTheButton0 = 0;
            this._internal_painInTheButton1 = 0;
            this._internal_painInTheButton2 = 0;
        }
        return testProperties;
    }());
    exports.testProperties = testProperties;
    var test = new testProperties();
    test._internal_thank = 0;
    test._internal_you = 0;
    for (var i = 0; i < 3; i++)
        test['painInTheButton' + i] = i;
    var totalThanks = test._internal_thank + 1;
    var totalForYou = test["_internal_you"] + 0;
    var button1Result = test._internal_painInTheButton1;
    var button2Result = test['painInTheButton' + 2];
    var button3Result = test['painInTheButton3'];
})(output);

Additional context
The two issues are...
1. 'painInTheButton' + i wont get renamed to '_internal_painInTheButton' + i, even if it did, terser wouldn't be able to rename that property to the mapped mangle.
2. 'painInTheButton3' doesn't exist on the object, since I only defined up to 2.

I would prefer these computed and invalid properties to throw an error when compiling, and if I want to ignore them have some option to do so. I would rather not find the bug through user report :(

The reason I would think the invalid property should error is to specifically review, and then can change to warn about it, or ignore it

[timocov]

I think that you don't use enabled noImplicitAny compiler option, right? This code can't be even compiled with enabled option: https://www.typescriptlang.org/play/#code/KYDwDg9gTgLgBAYwDYEMDOa42GmAFKCMYWASxzgG8AoOOuMAVwCMlSEsALFAOwGsAXHB6MAtsxJwAvHAAMAblr0mrdnACeERkJHjJMhUroq2HMClI8AkjwAqnYACFGMGBB6ydYiVGlzF9AwspgwW1nYOzq7uAIxeer4GAcrBauaWNvZOLm48AEzxPn6GAL7U1ABuKL7YuH48wADuWDj4hMRkOAAUAJSKtTAAdDDc-MX9rYOajOPUAGbQcF1VvqTjcGsAPHAAzPIbANQHPUYtuADaAOTp4VlRuZdwBxsAun6kiitYEDAoSPa8PiYGQDYajPhPOAxT7Vb6-JAAMWgAE0tH4Bldppc3s9DF9mDlYgAlHCMJDwEGTG6ZSKEnjQyqwgnRfIktBkilnGBXakRbIsx7PPIvGG+Zm5HZsjno1o8sI0-kS7HyIA.

[timocov]

I you'd like to disable renaming for some properties, you can mark them as @public with jsdoc (or any tag you set via config) and they won't be renamed - https://github.com/timocov/ts-transformer-properties-rename#publicjsdoctag

But I agree that it'd be nice to have some warnings/errors to notify that something could be broken.

[artfiedler]

I'm using, noImplicitAny=false

https://www.typescriptlang.org/play/?noImplicitAny=false&target=2&module=2#code/KYDwDg9gTgLgBAYwDYEMDOa42GmAFKCMYWASxzgG8AoOOuMAVwCMlSEsALFAOwGsAXHB6MAtsxJwAvHAAMAblr0mrdnACeERkJHjJMhUroq2HMClI8AkjwAqnYACFGMGBB6ydYiVGlzF9AwspgwW1nYOzq7uAIxeer4GAcrBauaWNvZOLm48AEzxPn6GAL7U1ABuKL7YuH48wADuWDj4hMRkOAAUAJSKtTAAdDDc-MX9rYOajOPUAGbQcF1VvqSF+v5wpHAAPHAAzPJbANTHPUYtuADaAOTp4VlRuTdwx1sAun6kiitYEDAoJD2Xh8TAyAbDUZ8V5wGI-ap-AFIABi0AAmlo-ANbtMbp83oZfswcrEAEo4RhIeDgyb3TKREk8OGVBHE6L5cloSnUy4wW50iLZdkvN55d7w3xs3L7TncrGtflhelC6V4+RAA

[artfiedler]

As for having the errors/warnings I think it would be ideal, because I can't think of a case where this would NOT cause an error in the output in production silently, when having a computed property at least. I think around line 129 in transformer.ts if its an element access expression and NOT a string literal throw a nice error :)

[timocov]

I'm using, noImplicitAny=false

Yeah, that's why you don't have any error. I think that it's really better to enable it, because it can avoid some hidden errors (this transformer rely on type system, and if you'll have any in some places - the tool won't rename them and your build could be broken).

I think around line 129 in transformer.ts if its an element access expression and NOT a string literal throw a nice error :)

Yeah, just created #10 for that.

So, what's we have at the end here (you need to use enabled noImplicitAny and we'll add warnings/errors in #10)?

[artfiedler]

In my project I have cases where I may use a computed property or a property that has a special character in it. It seems if noImplicityAny is turned on instead of simply doing test['salad-dressing'], I would then need to do(<any>test)['salad-dressing']and then run into the same issue?

[timocov]

I would then need to do(test)['salad-dressing']and then run into the same issue?

Any access to any type isn't modified, because we don't know whether a type is exported or not.

[artfiedler]

Isn't the type irrelevant? Only the property name, it shouldn't even be an anytype as it is just accessing the property test.salad and test[‘salad’] are equivalent

…

Sent from my Windows 10 device From: Evgeniy Timokhov Sent: Monday, June 29, 2020 10:27 AM To: timocov/ts-transformer-properties-rename Cc: Arthur Fiedler; Author Subject: Re: [timocov/ts-transformer-properties-rename] Computed or Invalidproperty names (#8) I would then need to do(test)['salad-dressing']and then run into the same issue? Any access to any type isn't modified, because we don't know whether a type is exported or not. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[timocov]

Isn't the type irrelevant?

Not actually - the tool tries to understand whether a property is exposes to the public (or declared in outside of the project) uses types:

ts-transformer-properties-rename/src/transformer.ts

Lines 276 to 317 in c38f3e7

	function isTypePropertyExternal(type: ts.Type, typePropertyName: string): boolean {
	if (type.flags & ts.TypeFlags.Object) {
	const objectType = type as ts.ObjectType;
	// treat any tuple property as "external"
	if (objectType.objectFlags & ts.ObjectFlags.Tuple) {
	return true;
	}
	}
	if (type.isUnionOrIntersection()) {
	const hasExternalSubType = type.types.some((t: ts.Type) => isTypePropertyExternal(t, typePropertyName));
	if (hasExternalSubType) {
	return hasExternalSubType;
	}
	}
	const symbol = type.getSymbol();
	if (symbol !== undefined) {
	const declarations = getDeclarationsForSymbol(symbol);
	for (const declaration of declarations) {
	if ((ts.isClassDeclaration(declaration) || ts.isInterfaceDeclaration(declaration)) && declaration.heritageClauses !== undefined) {
	const hasHeritageClausesExternals = declaration.heritageClauses.some((clause: ts.HeritageClause) => {
	return clause.types.some((expr: ts.ExpressionWithTypeArguments) => {
	return isTypePropertyExternal(typeChecker.getTypeAtLocation(expr), typePropertyName);
	});
	});
	if (hasHeritageClausesExternals) {
	return true;
	}
	}
	}
	}
	const propertySymbol = typeChecker.getPropertyOfType(type, typePropertyName);
	if (propertySymbol === undefined) {
	return false;
	}
	return [propertySymbol, ...splitTransientSymbol(propertySymbol, typeChecker)]
	.some((sym: ts.Symbol) => getSymbolVisibilityType(sym) === VisibilityType.External);
	}

Only the property name

But to detect the property of what type, we uses type system.

test.salad and test[‘salad’] are equivalent

Yes, they are and they both are handled by the tool in the same way (they should be).

it shouldn't even be an anytype as it is just accessing the property

And again - we need to know of what type the property is to handle renaming correctly (thus, for instance, one property could be internal one, another one could be public).

[artfiedler]

Makes complete sense, so I'll slowly work my project to make everything type specific. I had given the updated code a whirl and significantly more compression/mangling occurred, however I did receive some errors when first testing the resulting code. I'm going to caulk this up as an issue with the any type issue for now without investigation, but I'll be sure to follow up when I get my code compiling with out implicit any.

[artfiedler]

However, I was looking at a few issues I might run into and I think it would be ideal if any internal/private renamings were traced some to determine if they ever run into an inconsistent type and then abort renaming and generate a warning... Here are some cases I'm thinking...

Btw the client side of my webapp project is over 2MB generated before minification so for me converting from no property mangling to property mangling and verifying everything is going to be time consuming. So some safe guards to make sure I find any potential issues at compile/transform time is going to be key.

Source

export class Direction {
    public static Top = 1;
    public static Bottom = 2;
    public static UpsideDown = 3;
}

export class Directions {
    public static fromString(direction: string): Direction {
        return Direction[direction];
    }
}

console.log("Dynamic property via method", Directions.fromString("Top"));
var direction: string = "Top"; // Could be from string
console.log("Dynamic property reference via property", Direction[direction]);
var direction: string = (<any>{ direction: 'Top' }).direction; // Could be from lets say XMLHttpRequest response
console.log("Dynamic property reference via property", Direction[direction]);
console.log("Static property reference", Direction["Top"]); // Works

// From XMLHttpRequest response without defining a type structure
var test1: string = (<any>{ direction: 'Top' }).direction;
// From XMLHttpRequest response without defining a type structure
var test2: string = (<any>{ /** @public */ direction: 'Top' }).direction;
// From XMLHttpRequest response with type structure (could be defined as test data instead of received from actual XMLHttpRequest)
var test3: string = (<{ direction: string }><any>{ direction: 'Top' }).direction;
// From XMLHttpRequest response with type structure (could be defined as test data instead of received from actual XMLHttpRequest)
var test4: string = (<{ /** @public */ direction: string }><any>{ direction: 'Top' }).direction;

Output

(function(exports){
exports.Directions = exports.Direction = void 0;
    var Direction = (function () {
        function Direction() {
        }
        Direction._internal_Top = 1;
        Direction._internal_Bottom = 2;
        Direction._internal_UpsideDown = 3;
        return Direction;
    }());
    exports.Direction = Direction;
    var Directions = (function () {
        function Directions() {
        }
        Directions._internal_fromString = function (direction) {
            return Direction[direction];
        };
        return Directions;
    }());
    exports.Directions = Directions;
    console.log("Dynamic property via method", Directions._internal_fromString("Top"));
    var direction = "Top";
    console.log("Dynamic property reference via property", Direction[direction]);
    var direction = { _internal_direction: 'Top' }.direction;
    console.log("Dynamic property reference via property", Direction[direction]);
    console.log("Static property reference", Direction["_internal_Top"]);
    var test1 = { _internal_direction: 'Top' }.direction;
    var test2 = { _internal_direction: 'Top' }.direction;
    var test3 = { _internal_direction: 'Top' }._internal_direction;
    var test4 = { _internal_direction: 'Top' }._internal_direction;
})(FC);

Issues as I see them...

1. Constant String variable reference as quoted property not modified

   • Directions.fromString("Top") isn't modified to "_internal_Top"
   • var direction = "Top"; isn't modified as well

2. When a private object is cast to an any type, I would think this should either not rename the property automatically(raise warning in verbose mode) because their could be potential undetected usages or throw an error that /* @public */ should be used

   • var direction = { _internal_direction: 'Top' }.direction;
   • var test1 = { _internal_direction: 'Top' }.direction

3. Can't stop renaming of private object cast to any (there isn't a super valid case for these examples, but it could happen)

   • var test2: string = ({ /** @public */ direction: 'Top' }).direction;
   • var test4: string = (<{ /** @public */ direction: string }>{ direction: 'Top' }).direction;

4. This works, but should it work? Because it passed through the any time (more of a misc test case)

   • var test3: string = (<{ direction: string }>{ direction: 'Top' }).direction;

[timocov]

@artfiedler

Directions.fromString("Top") isn't modified to "_internal_Top"

Yeah, it looks like a bug, but only in case of different type of direction in fromString method: it shouldn't be a string, it should be something like keyof typeof Directions.

But even in this case there is issue with this code - it's easy to access to prototype property (because keyof typeof returns it). If you don't change it, you can easily return toString or any other function's property. So I'd say it should have a type 'Top' | 'Bottom' | 'UpsideDown' and in this case it's bug.

var direction = "Top"; isn't modified as well

The type of direction is string and we just can't understand that it's 'Top' | 'Bottom' | 'UpsideDown' and that's type is related to Direction. It's similar to previous one.

var direction = { _internal_direction: 'Top' }.direction;
var test1 = { _internal_direction: 'Top' }.direction

It's tricky and comes from TypeScript type system. The original code looks like (<any>{ direction: 'Top' }).direction. So, the first you've declared an object with property direction: 'Top'. We just don't know whether you're going to case it to something or not, at this moment it has type __type or so (TypeScript's internal) and it isn't any. Then you cast it to any and access to property direction - here we know that it's any and drop any renaming from it.

I don't even know how to identify such cases to warn about them actually.

var test2: string = ({ /** @public */ direction: 'Top' }).direction;

It looks like a bug, I'll try to fix it.

This works, but should it work? Because it passed through the any time (more of a misc test case)
var test3: string = (<{ direction: string }>{ direction: 'Top' }).direction;

It works because it renamed both direction properties to _internal_direction? The explanation is similar to the first case above.

var test4: string = (<{ /** @public */ direction: string }>{ direction: 'Top' }).direction;

The same as the first one about "late type binding".

Overall, I agree that there might be cases where it'd be better to have warnings/errors or even just work, but often we just "faced" limitation of TypeScript type system and how it works. All cases I've seen (from your examples or in my projects) could be fixed by putting types for them. Some of practices couldn't be just used due their nature (like accessing to a random object's property, using any, casting to any somewhat, etc).

Advanced optimizations require put some limitations to your project, what you can use and what can't, what can break your project easily. If you'd like you use TypeScript in JavaScript-like style (don't write types, don't specify all "inbound" and "outbound" types of variables/functions/classes), I'd say that you can't use this transformer (like you can't use google closure compiler for similar reasons - it also requires you write the code in the way it understand rather than you'd like to write).

[timocov]

var test2: string = ({ /** @public */ direction: 'Top' }).direction;
It looks like a bug, I'll try to fix it.

Actually it seems that it's incorrectly written JSDoc (or TSDoc?). If you write it in the following it will works as expected and won't be minified:

var test2: string = ({ 
	/** @public */
	direction: 'Top'
}).direction

[timocov]

But even in this case there is issue with this code - it's easy to access to prototype property (because keyof typeof returns it). If you don't change it, you can easily return toString or any other function's property. So I'd say it should have a type 'Top' | 'Bottom' | 'UpsideDown' and in this case it's bug.

JS is very dynamic language, which allows you doing a lot of crazy stuff, which will be grammatically correct, could be compiled with TypeScript, but cannot be easily (or even totally) optimized in any way.

As I said, advanced optimizations require put some limitations to your project. I guess that it'd be one of them. You can use enums instead, or mark this class as public to avoid it's minification.

I just tried to fix it, and there are a lot of other cases which StringLiterals we should "rename" and which shouldn't. In other hand, we need to rely on other tools (like terser) which will minify the code afterwards. And it seems that it doesn't work with random string literals, so I don't think that we should fix it either (we can't rename more than terser/uglifyes can minify).