Undecorated Classes: Migration Checklist

Owner: Alex R
Date: 6/26/19

Related design docs:

• Original problem description
• Design for Ivy code changes

Design

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

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

2. 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:

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

2. 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:

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

2. 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:

1. knows how to generate imports, so it can handle cases where the schematic would have to bail out.

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

• Design doc for migration. Should include:

  • Why create a migration? (Important)

    • Why can't we just "fix" this?

    • How do we know it's impactful enough that we need a migration?

  • How will the schematic work?

  • What does this mean for libraries?

  • What about applications using non-migrated libraries?

  • Should this schematic be used externally or internally? In which version?

  • Is there something that we should deprecate? In which version?

  • How will we prevent backsliding?

  • Any open questions?

• Post doc in #fw Slack channel for feedback

• Post doc in #tools Slack channel for feedback

• Confer with #devrel Slack channel for feedback

• Present in framework sync to answer Qs, get buy-in from team

Approval

• Kara
• Igor
• Misko

After Approval

• Add to appropriate list of migrations (see v9 list)

• Schematic implementation

  • Create a JIRA for schematic implementation and send to Kara

    • Link: https://angular-team.atlassian.net/browse/FW-1421

  • PR:

• Internal migration in G3

  • Create a JIRA and send to Kara

    • Link: https://angular-team.atlassian.net/browse/FW-1423

  • [If applicable] LSC process

    • Create a JIRA for LSC design doc and send to Kara

      • Link: https://angular-team.atlassian.net/browse/FW-1424

• Migration guide

  • Create a JIRA and send to Kara

    • Link: https://angular-team.atlassian.net/browse/FW-1425

  • Let docs team / Brandon know (needs docs review)

• Communicate new schematic with the rest of team

  • Present in team meeting

• Test schematic in RC period