Undecorated Base Classes for Directives

Problem

In some applications, users declare base classes for directives & components:

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

@Component({...})
export class FooCmp extends FooBase {
  // Constructor inherited from FooBase.
}

View Engine, when compiling FooCmp, takes advantage of its global knowledge to discover the constructor parameters needed for dependency injection of FooCmp, and generates the NgFactory for it accordingly.

In Ivy, this pattern falls apart:

  1. FooCmp has an ngComponentDef with a factory that relies on a getInheritedFactory operation at runtime to extract a DI factory from FooBase.

  2. FooBase has no Ivy definition and thus no factory of its own, so getInheritedFactory returns a default factory of (t) => new t().

  3. FooCmp is constructed via factory(FooCmp) which ends up executing as new FooCmp(). Thus, no dependencies are injected at all.

  4. FooCmp.vcr is undefined, c̷̮̅â̸̻u̶̫͝s̶̝͂i̷̝͝n̸͔̂g̴͎͊ ̵̹̔c̶̟̈h̴͔̿ã̸͎o̷̙͊s̴̡͗ ̴͝ͅa̴̰͋t̸̢͠ ̶͕̅ṙ̵̫u̷̮̍n̶̤̓t̶̜̋i̴̙̿m̸͉̎e̷̡͒.

See more about how View Engine behaves in design doc here.

Proposal

Ivy's model is that if a constructor is inherited, the base class should always have a definition. Thus, it should have a decorator:

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

@Component({...})
export class FooCmp extends FooBase {
  // Constructor inherited from FooBase.
}

This results in ngDirectiveDef being generated for FooBase, which means getInheritedFactory returns a correct factory.

selector on @Directive becomes optional

Currently, if you write @Directive({...}) and don't specify a selector, you get a build error (if AOT) or runtime error (if JIT). It's optional in the typings because @Component extends @Directive and selectors for components are optional by design.

This proposal changes the meaning of @Directive without a selector, interpreting it as a directive which cannot be instantiated, but can still contain metadata (host bindings, etc) which is inherited.

The meaning of a selector-less @Component does not change. It does not make sense to mark an abstract base class @Component as components must have templates, which does not make any sense for an abstract class.

The View Engine compiler would be modified to accept selector-less @Directives.

selector and @NgModule interaction

Currently, there is also an error in View Engine if a @Directive is not a part of the declarations of any @NgModule. This makes sense - a directive that isn't included in any module can't be used.

This check would be narrowed so that it only applies to a directive with a selector. That is:

  1. If the @Directive has a selector, it must be declared in an @NgModule, and it's an error if it's not.
  2. If the @Directive has no selector, it must not be declared in an @NgModule. If it is, an error is also produced.

Rule #2 exists because a directive without a selector cannot be used in a template anyway, and so including it in the compilation scope of a component makes no sense.

The View Engine compiler would be modified to support the above checks.

Alternatives considered

@Directive({abstract: true})

In this alternative, a flag would be added to the @Directive annotation (and possibly @Component) indicating that the usage is abstract. The View Engine compiler would be updated to respect this annotation with regard to producing the above errors.

Pros:

  • a more explicit syntax

Cons:

  • fixing the typings would be a major undertaking

Ideally, the typings would specify that abstract: true must not have a selector field for @Directive. This would be a lot of work to roll out - Directive is an interface currently, but would need to be changed to a type union, and that change propagated through the rest of the codebase.

  • the flag adds no extra information

It only really serves as a sanity check - the presence or absence of the selector is already enough to tell how the directive can be used.

@AbstractDirective

Adding a new annotation for base classes is also an idea. It would be trivial to implement in View Engine as long as the annotation does not have any behavior associated with it. Since View Engine already supports these base classes natively, this isn't a problem.

However, @Directive traditionally allows you to specify metadata like host bindings, etc. within the decorator. In base classes, that metadata can also be added using decorated fields (@HostBinding). Conceptual symmetry would dictate that even for abstract directives, metadata could also be specified in the decorator. Implementing this for View Engine would be a big change.

Pros:

  • also an explicit syntax

Cons:

  • a major change to the View Engine compiler to implement with full metadata

Recognizing inheritance within the compilation unit

The Ivy compiler can statically understand inheritance structures as long as they're within the current compilation (not a base class from a library on NPM). It could write a correct factory for the derived class (FooCmp in example) based on the constructor parameters of the base class.

Pros:

  • this makes the change non-breaking

Cons:

  • it introduces another boundary in Angular where stuff works until you refactor it into different libraries
  • increased mental model complexity

Notes from meeting

  • Leaning toward requiring @Directive annotation for any class with "Angular" behavior (e.g. inputs, outputs, host bindings, queries, or DI) that is extended by a directive / component. Otherwise, you need the annotation for DI but not for other Angular behavior, which is weird.
  • This would mean that we need to do a larger migration to add @Directive() decorators to any undecorated base classes.
  • If you try to use a param / field decorator without class decorator, compiler will throw an error.
  • If you try to inherit the constructor from an undecorated class, the compiler will throw an error.
  • If a child class is not inheriting a constructor (has its own constructor) and the parent class is not decorated, it's ok.
  • Base defs are no longer a thing (only directive defs).
  • TODO: Alex to write up migration plan
Select a repo