Removing ngBaseDef by migrating classes

Owner: Alex R
Date: 7/25/19

ngBaseDef

View Engine in both JIT and AOT would happily accept class structures like the following:

class Base {
  @Input()
  foo: string;
}

@Directive({...})
class Dir extends Base {
  ngOnChanges(): void {
    // notified when bindings to [foo] are updated
  }
}

Essentially, undecorated base classes which have decorated fields (@Input, @ViewChild, etc) work perfectly in View Engine.

In the Ivy model this causes problems, as there is no annotation on Base which would generate an Ivy definition to carry the information about inputs, outputs, queries, or host bindings. We've worked around this by generating a special ngBaseDef field when an otherwise undecorated class has decorated fields.

The plan

The goal is to remove the need for ngBaseDef by enforcing that every undecorated class with decorated fields has an @Directive() annotation, and thus will receive an ngDirectiveDef which carries the information about those fields. This is a straightforward transformation.

The "Undecorated Classes" migration

It's important to note that this case is not caught by the Undecorated Classes migration, because that migration is exclusively concerned with inheritance of constructors.

Design

Why can't we just fix this?

Arguably, ngBaseDef is "just fixing this". It allows the runtime to work without the need to migrate user code.

We want to remove it for a couple reasons:

  1. Supporting this case makes the Angular mental model more complex.

The rule we're trying to apply with Ivy is that if it's a class with Angular behavior, it must have at least one Angular decorator.

The fact that this case worked in the past is more a fluke of TypeScript's behavior than anything else. Removing it will make Angular more intuitive for users.

  1. Supporting this case with ngBaseDef increases the runtime's complexity.

ngBaseDef adds an extra layer of complexity for the InheritDefinitionFeature which handles inheritance at runtime. Additionally, it requires extra machinery for JIT support in the field decorators. Removing it will significantly simplify the runtime model for inheritance.

  1. ngBaseDef was never a complete solution anyway.

It never handled the case of constructor inheritance, and so some base classes do require @Directive(). Therefore it was a flawed solution from the start.

How is "Classes with Angular behavior" defined?

There are two independent questions here.

  • What's technically possible (i.e. what classes would technically need to be decorated)?
  • What DX should we offer (i.e. what's the mental model)?

As far as what's technically possible, there are cases where an intermediate class
can get away with not having Angular definitions (not requiring a decorator), if it:

  • is itself abstract/does not need to be instantiated by Angular directly
  • has no constructor (isn't using DI)
  • has no Angular decorated fields

In those cases the classes could be considered "transparent" to the Angular runtime - attempts to resolve a factory definition will return the factory definition of its parent (if one exists), which is suitable for instantiating the class still, as will attempts to inherit any Angular metadata.

As for the more important question "what should the DX we offer be?", a rule of "if it extends from an Angular decorated class, it needs a decorator of its own" is vastly simpler and easier for users to apply than "well, if it has Angular fields than it needs a decorator, otherwise it doesn't, unless it has a constructor in which case it does…"

Link to the original discussion

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

This is a common enough pattern in g3 that we developed the ngBaseDef solution in the first place.

How will the schematic work?

The schematic is pretty simple.

  1. Scan classes in the compilation.
  2. If the class is not decorated at the top level but has Angular decorated fields, add @Directive() to the class.

Angular decorated fields include:

  • @Input
  • @Output
  • @HostBinding
  • @HostListener
  • @ViewChild / @ViewChildren
  • @ContentChild / @ContentChildren

It does not include param decorators for dependency injection.

What does this mean for libraries?

Libraries need to update in the same way.

What about applications using non-migrated libraries?

Since libraries face the same challenge, ngcc via its migration framework will have to perform the same change for libraries, and add ngDirectiveDef to them.

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

Both. Externally, it should probably be in 9.0

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

If we make this change, it would mean Ivy implicitly deprecates the ability to use undecorated base classes.

How will we prevent backsliding?

We should make this an error in the View Engine compiler to ensure code is compatible with Ivy in the future.

Any open questions?

None.

Appendix for v10

This migration has landed in version 9 of Angular. Now with Angular v10, we plan on removing compatibility code in ngtsc that allowed the old pattern to still work.

We want to remove this compatibility code now so that developers are forced to apply @Directive where needed (as per backsliding prevention section). The goal is that developers quickly adopt the more consistent and easier mental model of where to apply @Directive.

We will keep the compatibility code specifically for ngcc so that View Engine libraries can still be consumed. In ngtsc we will print a helpful error that instructs developers to add an abstract directive decorator. Tracked with https://angular-team.atlassian.net/browse/FW-2130.

It's also worth noting that this migration initially has been only concerned with
undecorated classes that have decorated class members w/ Angular decorators (e.g. @Input()). Now with the planned removal of the ngtsc compatibility code, we would also need to migrate classes that use Angular lifecycle hooks. This matches with the compatibility code, but has not been initially captured in the migration or in this plan.

  • 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?

    • Link: https://hackmd.io/vuQfavzfRG6KUCtU7oK_EA?view

  • 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

  • Internal migration in G3

    • Create a JIRA and send to Kara

      • Link:

    • [If applicable] LSC process

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

        • Link:

  • Backsliding

    • Create a JIRA for implementation of the backsliding plan and send to Kara

      • Link:

  • Migration guide

    • Create a JIRA and send to Kara

      • Link:

    • 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

Select a repo