# Undecorated Classes: Migration Checklist

Owner: Alex R
Date: 6/26/19

#### Related design docs:
- [Original problem description](https://hackmd.io/Ihp4YWMoQ1e1nbSbg7HStA?view)
- [Design for Ivy code changes](https://hackmd.io/wuNqEgYCQc2tZBr0dpaWHQ?view)

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

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

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](https://hackmd.io/wuNqEgYCQc2tZBr0dpaWHQ?view)).

@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

- [x] Design doc for migration. Should include:

- [x] Why create a migration? (**Important**)
- [x] Why can't we just "fix" this?

- [x] How do we know it's impactful enough that we need a migration?

- [x] How will the schematic work?

- [x] What does this mean for libraries?

- [x] What about applications using non-migrated libraries?

- [x] Should this schematic be used externally or internally? In which version?

- [x] Is there something that we should deprecate? In which version?

- [x] How will we prevent backsliding?

- [x] Any open questions?

- [x] Post doc in #fw Slack channel for feedback

- [x] Post doc in #tools Slack channel for feedback

- [x] Confer with #devrel Slack channel for feedback

- [x] Present in framework sync to answer Qs, get buy-in from team

## Approval

- [x] Kara
- [x] Igor
- [ ] Misko

## After Approval

- [x] Add to appropriate list of migrations (see [v9 list](https://docs.google.com/spreadsheets/d/1jqeXZ85N3SR5uTSndIfBuGebLgeL0zAnHH63290AKNM/edit#gid=0))

- [x] Schematic implementation

- [x] Create a JIRA for schematic implementation and send to Kara

- [x] Link: https://angular-team.atlassian.net/browse/FW-1421

- [ ] PR:

- [ ] Internal migration in G3

- [x] Create a JIRA and send to Kara

- [x] Link: https://angular-team.atlassian.net/browse/FW-1423

- [ ] [If applicable] LSC process

- [x] Create a JIRA for LSC design doc and send to Kara

- [x] Link: https://angular-team.atlassian.net/browse/FW-1424

- [ ] Migration guide

- [x] Create a JIRA and send to Kara

- [x] 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