Undecorated Classes: Migration Checklist

Owner: Alex R
Date: 6/26/19

Design

This proposal discusses migration of inheritance patterns which are problematic in Ivy. There are two kinds of such patterns:

Inheritance of a constructor from a base class which is not decoratored.

export class BasePlain {
  constructor(private vcr: ViewContainerRef) {}
}

@Directive({selector: '[blah]'})
export class DerivedDir extends BasePlain {}

This is hereafter referred to as the "Undecorated Parent" pattern.

Inheritance from a decorated to an undecorated class.

@Directive({selector: '[blah]'})
export class BaseDir {
  ...
}

export class DerivedPlain extends BaseDir {}

This is hereafter referred to as the "Undecorated Child" pattern.

Both of these cases are somewhat problematic in View Engine JIT mode if no other decorators are found on the class, as the TypeScript compiler does not emit the relevant decorator metadata. In AOT mode (for example, all of g3), the compiler handles these cases just fine.

Why a migration?

The tl;dr answer is "locality".

Ivy's model is that the Angular behavior of a directive is described by its ngDirectiveDef (or ngComponentDef in the case of a component).

Undecorated Parent

For the "undecorated parent" case, when compiling a DerivedDir class which extends an undecorated BasePlain class, the compiler needs to generate a ngDirectiveDef for Derived. In particular, it needs to generate a factory function that creates instances of DerivedDir.

If DerivedDir has a constructor, this is trivial. The factory function will invoke new DerivedDir(...) with injected constructor parameters, and the BasePlain superclass is irrelevant.

If DerivedDir has no constructor, then we have a problem. The compiler has two choices:

Figure out the parameters to the constructor, and write a factory that calls new DerivedDir(...) and injects them.

This is a locality violation - the compilation of DerivedDir shouldn't depend on metadata about BasePlain, which may come from an upstream library and may be changed in a future version of that library.

Delegate construction of the instance to the factory for BasePlain, which has the constructor parameters.

Another way of putting this is that if the constructor is inherited, the factory should be too. Ivy is set up to inherit this way, but this requires that Base have a factory to begin with.

Undecorated Child

In the undecorated child case, the problem is more subtle. Consider the BaseDir/DerivedPlain directive pair, and assume that DerivedPlain is ultimately used in a component Cmp's template. Cmp will then have an ngComponentDef:

class Cmp {
    static ngComponentDef = defineComponent({
        ...,
        directiveDefs: [DerivedPlain.ngDirectiveDef],
    });
}

Ivy records that Cmp makes use of DerivedPlain by linking its ngDirectiveDef in Cmp.ngComponentDef.directiveDefs. Since DerivedPlain has no ngDirectiveDef property (by virtue of not being decorated), that read falls through to the prototype at runtime. As a result, BaseDir.ngDirectiveDef is recorded in Cmp's directiveDefs. Thus the information that DerivedPlain exists at all is lost to the runtime, which will instead incorrectly create instances of BaseDir.

Impact

We unfortunately do not have good numbers on the usage of inheritance in Angular. There are mitigating factors:

Externally, a lot of the problematic inheritance patterns don't work in JIT. This cuts down on the number of likely cases, as developers will not write code that doesn't work in Angular's dev mode.
Internally, there have been a medium number of occurrences of these issues. Internally JIT is not really used, so the mitigating effect of point #1 don't apply.

Here is a TAP from a CL that adds a compiler error for undecorated directives: https://test.corp.google.com/ui#id=OCL:256186366:BASE:256186854:1562087174458:592e6647

It appears to affect a good chunk of Google3.

Schematic Design

This section focuses on the external schematic. Internally, the situation is a bit more complicated due to the division of targets. Both versions suffer from an issue of detecting when classes in dependencies are decorated.

Detection within the compilation unit

This is the most likely case externally. Within a given compilation unit (tsconfig.json) file, it's possible to construct a TypeScript ts.Program and discover any decorated classes that inherit from a base class. This chain can be walked to identify classes that either:

Are directives/components, but inherit a constructor from a class without a Directive/Component decorator (undecorated parent), or
Do not have a decorator, but inherit from a class which does (undecorated child)

In the first case, the schematic can apply an @Directive() annotation to the parent class (without a selector). The compiler/runtime will support this (see design doc).

@Directive()
export class BasePlain {
  constructor(private vcr: ViewContainerRef) {}
}

@Directive({selector: '[blah]'})
export class DerivedDir extends BasePlain {}

In the second case, things are a bit trickier. The schematic will determine if metadata can be copied over.

For example, in this hierarchy:

@Component({
  selector: 'base-class',
  templateUrl: './base-class.html',
})
export class Base {}

export class Derived extends Base {}

None of the metadata for Base depends on imports, local variables, etc. so it can be cleanly copied to Derived (adjusting the paths if necessary).

@Component({
  selector: 'base-class',
  templateUrl: './base-class.html',
})
export class Base {}

@Component({
  selector: 'base-class',
  templateUrl: './base-class.html',
})
export class Derived extends Base {}

Problematic metadata

Sometimes this won't be possible. Consider this base class:

import {SomeService} from './path/to/service';

export const TEMPLATE = '....';

@Component({
  selector: 'base-class',
  providers: [SomeService],
  template: TEMPLATE,
})
export class Base {}

Here, the metadata contains references to other symbols, some of which are imported and some which are exported from this file. If this metadata were copied to the Derived class, imports would need to be added for these symbols. Adding such imports is possible, but non-trivial due to concerns like path mapping.

In rare cases like this, the schematic should add a decorator to the derived class with any metadata fields (e.g. selector) that can be copied, as well as comments for the fields that can't. For example:

@Component({
  selector: 'base-class',
  // The following fields were copied from `Base`, but
  // imports need to be added manually. Please add any
  // required imports and uncomment the below lines:
  // 
  // providers: [SomeService],
  // template: TEMPLATE,
})
export class Derived {}

Inheritance hierarchies spanning dependencies

In general, the question of "is this class an Angular directive/component" is hard for a schematic to answer. For classes in the compilation unit, with .ts source available, it's possible to detect the presence of an Angular decorator. In a .d.ts file, it's not so easy. To make this work, the schematic should use the View Engine compiler to load the program, allowing use of the metadata loaded from .metadata.json files.

For case 1 (undecorated parents), if a component/directive in the compilation unit inherits a constructor from an undecorated base class, and the base class is from a library (.d.ts file), then the migration needs to add a comment to the derived class explaining that an explicit constructor needs to be declared. This case should be very rare.

For case 2 (undecorated child), it should be possible to look at the metadata for the base class and make a determination about whether it can be automatically generated per above.

ngcc

These transformations will also need to be applied by the Angular compatibility compiler, ngcc.

ngcc's job will be a bit simpler than the schematic, since it:

knows how to generate imports, so it can handle cases where the schematic would have to bail out.
has a much easier time of detecting whether a class is a directive/component, thanks to Ivy.

Backsliding

Both of these patterns will become diagnostic errors in the Ivy compiler, ngtsc.

G3

Short term: After the schematic is run internally, we can create a local mod to cause View Engine to throw an error if a directive is not properly decorated.

Long term: In g3 we will eventually run the Ivy compiler in parallel to produce diagnostics.

Deprecations

No deprecations necessary - the old behavior is implicitly deprecated as it's not supported by Ivy.

Checklist

Approval

Kara
Igor
Misko

After Approval

Igor Minar

2019/07/02 00:10:37

what if the child adds and @Input or @Output? or @HostBinding or @HostListener? don't we need to create a new factory that will take these decorators specified on the child into account? (Edited)

2019/07/02 00:14:14

It's not clear to me if this is a statement of the problem or of the proposed solution. Wouldn't falling through to `BaseDir. ngDirectiveDef` break locality? how will the compiler know to look at BaseDir? (Edited)

2019/07/02 00:18:10

this seems like a ton of work for very little impact. (Edited)

2019/07/02 00:19:40

Do we know of any undecorated library out there on npm? I HIGHLY doubt it. My preference would be to not implement this part until we find that this is affecting lots of users AND we realize that we can't get the library owners to decorate their base classes. (Edited)

2019/07/02 00:22:26

The ngcc part is light on details, I think we should not try to deal with undecorated code in libraries unless we find that we absolutely have to because of how common this pattern is (I don't think it's common). (Edited)

Kara

2019/07/02 17:27:26

Those decorators are implemented through other properties on the directive def (they aren't part of the factory function) (Edited)

2019/07/02 17:29:28

I think this is a problem statement. It falls through to BaseDir due to inheritance / prototype chain at runtime, not because we are explicitly replacing the reference at compile time. That's a problem because we don't have access to the DerivedPlain directive's type to correctly invoke the factory function (Edited)

Alex Rickabaugh

2019/07/02 17:32:48

Yep, it's the problem. At runtime the read falls through to the prototype, and so the runtime receives no information that `DerivedPlain` existed at all. I've clarified the statement to that effect. (Edited)

2019/07/02 17:35:09

We did this for static queries, so I don't think it's a huge amount of work. We can reuse the same logic for this schematic. I agree this case is probably rarer, though. (Edited)

2019/07/02 17:40:08

For ngcc that case is actually really trivial, because it'll first compile the library for Ivy and thus it knows trivially whether a class is decorated or not. As for the lightness of the section, I think it's sufficient to state that existing libraries will be supported by ngcc, which will apply the same transformation when upgrading them to Ivy. The actual design of how to do this in ngcc is out of scope. (Edited)

Paul Gschwendtner

2019/07/03 08:40:39

I also think that this will be quite some work. Respecting the metadata files by leveraging the Angular AOT compiler seems do-able (might be slightly similar to what we did for the static-queries) Though I'm definitely more worried about the copying of decorators. People think that adding a simple decorator is trivial, but it always turns out more complex than it initially looks. i.e. managing imports can become an effort (this one is already done; can be re-used) (Edited)

2019/07/03 08:41:30

Not saying the schematic is not doable, but we definitely need to be aware that it can take up more time. From my previous experience with update schematics.. there will always appear stuff that turns out more difficult and time consuming. Wondering if we need a implementation plan as part of the general migration checklist to estimate efforts. (Edited)