Use goog.reflect.object when downleveling property decorators

[robwormald]

In NgRx, we use decorators in a similar fashion to Angular - we "mark" class properties using a decorator, so that at runtime we can wire up the application logic:

import {Effect, Actions} from '@ngrx/store'

export class MyEffects {
  constructor(private actions: Actions){}
  @Effect() someProperty:Observable<any> = this.actions.switchMap(doSomeWork);
  @Effect() someOtherProperty:Observable<any> = this.actions.flatMap(doOtherWork);
}

These classes are then passed to the Effect library as a multi-provider:

@NgModule({
  imports: [ EffectsModule.forRoot([ MyEffects ])
})

and when the application launches, internally, we inspect those classes, and retrieve the marked properties (which are Observables), and subscribe to them:

const observables = getTheListOfSpecialPropertiesSomehow(MyEffects);
observables.forEach(obs => obs.subscribe(updateApplicationState);

Outside of Google, we do this with Reflect at runtime, and use decorators as decorators: https://github.com/ngrx/platform/blob/master/modules/effects/src/effects_metadata.ts#L38-L52 which all works fine (with the exception of this requiring the reflect-metadata polyfill, which is soon to be unnecessary in Angular core...)

Such usage of decorators is disallowed in google3, and so recently we implemented the @Annotation JSDoc tag, which "downlevels" the decorator to a static class property:

//output after tsickle/ngc

export class MyEffects {
  constructor(private actions: Actions){}
  someProperty:Observable<any> = this.actions.switchMap(doSomeWork);
  someOtherProperty:Observable<any> = this.actions.flatMap(doOtherWork);
}
MyEffect.propDecorators = {
  someProperty: [ { type: Effect, args: [] }],
  someOtherProperty: [ { type: Effect, args: [] } ]
}

This removes the dependency on Reflect (great!) but unfortunately, does not play nicely with closure compiler advanced optimizations, because post-property mangling, the property names captured in the downleveled annotation no longer match the (mangled) property names.

It turns out that Closure Compiler has the ability to handle this scenario - using the goog.reflect.object API mentioned in the last paragraph here - https://github.com/google/closure-compiler/wiki/Type-Based-Property-Renaming. That seems to indicate we can have our cake (using annotation-style decorators from third parties) and eat it too (maintain full closure optimizations).

Other options discussed w/ @mprobst include having developers handle this case themselves (eg, by returning an array of the special properties themselves) but that adds a potential footgun and removes some of the declarative nature of NgRx.

Adding this as a transform should work for all? @Annotation propDecorator usages and shouldn't require any special handling, as far as I can tell.

[mprobst]

Correct syntax would be:

MyEffect.propDecorators = goog.reflect.object(MyEffect, {
  someProperty: [ { type: Effect, args: [] }],
  someOtherProperty: [ { type: Effect, args: [] } ]
});

[robwormald]

Okay, so I implemented this locally and it works well - it allows full-fledged property renaming (meaning individual effects don't need the @export tag cc @ukrukarg) though it does require the /** @nocollapse */ tag to be added. Ideally, nocollapse should be added only if > 0 properties need to be retained for runtime.

I also discovered that today, tsickle actually generates quoted properties:

MyEffect.propDecorators = {
  'someProperty': [ { type: Effect, args: [] }],
  'someOtherProperty': [ { type: Effect, args: [] } ]
};

Changing that to generate non-quoted props:

MyEffect.propDecorators = {
  someProperty: [ { type: Effect, args: [] }],
  someOtherProperty: [ { type: Effect, args: [] } ]
};

appears to solve this issue as well, without the need for goog.reflect.object, though I'm not sure if that's simply by chance rather than a feature. Given these are property decorators, I can't think of a case where you'd need to quote the property name (?), so perhaps removing the quotes is Good Enough?

Inlining the goog.reflect.object call seems to be the most resilient approach, but it adds a runtime dep on goog.reflect, so maybe that should be behind a flag? Thoughts @mprobst ?

[mprobst]

I'm around this week, let's talk in person.

[robwormald]

Update on this: @mprobst is concerned that arbitrarily adding nocollapse to propDecorators could balloon code size (it would retain all such decorators, even Angular's @Input/@Output decorators which are definitely not needed at runtime.

Agreed to add a new JSDoc annotation @RuntimeAnnotation, which would denote that (like @Annotation) it should be downleveled, but unlike @Annotation it would need to be available at runtime (and thus needs to have @nocollapse added to it...)

We'll add this to the @Effect decorator, and tsickle will check if any of the decorators are marked as such, and we'll only emit the propDecorators that are marked @RuntimeAnnotation

Outstanding issue with this approach is that we really only want to do this for internal projects (that is, not the stuff shipped to NPM) as they still need to exist for JIT clients.