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

Method decorators in TypeScript break Closure Compiler minification #241

Closed
btraut opened this issue Oct 6, 2016 · 11 comments
Closed

Method decorators in TypeScript break Closure Compiler minification #241

btraut opened this issue Oct 6, 2016 · 11 comments

Comments

@btraut
Copy link

btraut commented Oct 6, 2016

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
Copy link
Contributor

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
Copy link
Contributor

rubenlg commented Jul 3, 2017

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
Copy link
Contributor

evmar commented Aug 20, 2019

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

@evmar evmar closed this as completed Aug 20, 2019
@rubenlg
Copy link
Contributor

rubenlg commented Aug 21, 2019

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
Copy link
Contributor

evmar commented Aug 21, 2019

@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.

@evmar evmar reopened this Aug 21, 2019
@rictic
Copy link
Contributor

rictic commented Aug 21, 2019

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
Copy link
Contributor

evmar commented Aug 21, 2019

Great, thanks.

@evmar evmar closed this as completed Aug 21, 2019
@evmar
Copy link
Contributor

evmar commented Aug 21, 2019

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
Copy link
Contributor

rictic commented Aug 21, 2019

+1

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

@marcfallows
Copy link
Member

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
Copy link
Contributor

evmar commented Aug 22, 2019

@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.

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

6 participants