angular / tsickle

Tsickle — TypeScript to Closure Translator
MIT License
896 stars 110 forks source link

Method decorators in TypeScript break Closure Compiler minification #241

Closed btraut closed 5 years ago

btraut commented 8 years ago

I'm extending this discussion over here from my Closure Compiler issue. The summary is as follows:

I have an AuthStore class that declares a getUser method that has an @autoSubscribe decorator. TypeScript compiles this by adding this after the class definition:

__decorate([
       resub_1.autoSubscribe
], AuthStore.prototype, "getUser", null);

After minification via Closure Compiler, my getUser is named d, but the call to __decorate still contains "getUser". This obviously fails.

@mprobst and @ChadKillingsworth suggest using goog.reflect to solve the issue like so:

__decorate([
       resub_1.autoSubscribe
], AuthStore.prototype,
goog.reflect.objectProperty("getUser", AuthStore.prototype),
null);

Is it possible that tsickle could extend support to method decorators to emit something like this?

alexeagle commented 7 years ago

If you still have __decorate calls in your JS output you'll have a bad time - these are not tree-shakable by closure compiler. ngc will use tsickle as a pre-processor, turning your decorators into static properties on the class. These in turn have @nocollapse to prevent a closure bug.

rubenlg commented 7 years ago

Not every project out there using tsickle also uses angular, which means that not everyone will run their code through ngc. As a workaround, developers can annotate their custom decorators with @ExportDecoratedItems, which will prevent closure renaming for annotated properties.

evmar commented 5 years ago

Our plan: tsickle doesn't support decorators. Angular does its own thing with decorators, separate from tsickle.

rubenlg commented 5 years ago

Polymer 3 is a heavy user of decorators, and is written in Typescript.

Does this mean tsickle won't be usable on Polymer projects?

It would help to clarify what "tsickle doesn't support decorators" means in practice.

evmar commented 5 years ago

@rictic What should tsickle be doing about polymer 3 decorators? I think the answer is "nothing", but I'm not sure what it means for this issue.

rictic commented 5 years ago

The status quo is working today for Polymer + closure compiler. Be sure to pass --polymer_version=2 and the @export flags. With those in place, all direct properties and methods on Polymer elements are not renamed.

evmar commented 5 years ago

Great, thanks.

evmar commented 5 years ago

To restate the position here:

tsickle does not explicitly support decorators, so Closure will do what it does with them. tsickle will not make decorators correct in the presence of renaming, so that falls on user code. Our position is that you should not use decorators, as they have been dropped by TS upstream as well (there are open bugs in the codegen for them that upstream will not fix).

rictic commented 5 years ago

+1

Polymer and LitElement users having issues with decorators + tsickle can reach out to me or others on the Polymer team.

marcfallows commented 5 years ago

as they have been dropped by TS upstream as well (there are open bugs in the codegen for them that upstream will not fix).

@evmar could you provide links to some of those bugs?

evmar commented 5 years ago

@marcfallows I don't remember the bug offhand, but it was in some pattern of how Angular used decorators where TS would use the wrong reference. I think this is why they moved to a model of removing the decorators before TS compiles them.

You can look through https://github.com/Microsoft/TypeScript/issues?utf8=✓&q=is%3Aissue+is%3Aopen+decorator+label%3ABug+ if you want though.

I think I read that TS is interested their decorators following the spec, so I expect they will resume work on reimplementing them with the new different behavior once the spec is done.