[ivy/ngcc] Fails to inject optional dependency in Angular8 library

[PapaNappa]

🐞 bug report

Affected Package

The issue is caused by package @angular/core or ngcc

Is this a regression?

Yes, the previous version in which this bug was not present was: 8

Description

I have a library (still building with Angular8 for backwards compatibility) that has one main service Lib2Service:

@Injectable({ providedIn: 'root' })
export class Lib2Service {
  constructor(
      @Optional() private lib1: BaseLib1 | null,
  ) {}

  getValue() {
    if (this.lib1) return this.lib1.getValue();
    return -1;
  }
}

This Lib2Service takes an optional service BaseLib1.
The use case is to provide plugins to this service.

In this example, the 'plugin' BaseLib1 has only a single method:

export class BaseLib1 {
  getValue() {
    return 0;
  }
}

The Angular8 library also provides a default implementation Lib1Service:

@Injectable()
export class Lib1Service extends BaseLib1 {
  getValue() {
    return 1;
  }
}

The intended use in any app is then to provide a certain implementation of BaseLib1, e.g. like so using the default implementation:

@NgModule({
  declarations: [AppComponent],
  imports: [BrowserModule],
  providers: [
    Lib1Service,
    { provide: BaseLib1, useExisting: Lib1Service },
  ],
  bootstrap: [AppComponent]
})
export class AppModule {
}

Then, any component can use Lib2Service, like so:

@Component({
  selector: 'app-root',
  template: `
      lib service: {{lib2Service.getValue()}}
  `,
  styles: []
})
export class AppComponent {
  constructor(
      public lib2Service: Lib2Service,
  ) {
  }
}

This works fine when the app is also using Angular 8, but with Angular 9, Lib2Service is constructed with its lib1 parameter undefined.

If I add @Inject(BaseLib1) to the ctor parameter of Lib2Service, the dependency is injected correctly. However, this was not necessary with an Angular 8 app and breaks the existing version of the library.

🔬 Minimal Reproduction

I have reproduced the Angular 9 app at https://github.com/PapaNappa/Angular9-DI-Bug
It contains a .tgz of the Angular8 library, which is basically the three classes from the top of this post.

Steps to reproduce:

1. git clone https://github.com/PapaNappa/Angular9-DI-Bug
2. cd Angular9-DI-Bug
3. npm install
4. ng serve
5. Open the app
6. The app should output the value 1 from Lib1Service, put it actually prints -1 indicating that the optional dependency lib1 is not provided to Lib2Service.

🌍 Your Environment

Angular Version (library):

Angular CLI: 8.3.25
Node: 12.14.1
OS: win32 x64
Angular: 8.2.14
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.803.25
@angular-devkit/build-angular      0.803.25
@angular-devkit/build-ng-packagr   0.803.25
@angular-devkit/build-optimizer    0.803.25
@angular-devkit/build-webpack      0.803.25
@angular-devkit/core               8.3.25
@angular-devkit/schematics         8.3.25
@angular/cli                       8.3.25
@ngtools/webpack                   8.3.25
@schematics/angular                8.3.25
@schematics/update                 0.803.25
ng-packagr                         5.7.1
rxjs                               6.4.0
typescript                         3.5.3
webpack                            4.39.2

Angular Version (app):

Angular CLI: 9.0.1
Node: 12.14.1
OS: win32 x64

Angular: 9.0.0
... animations, common, compiler, compiler-cli, core, forms
... language-service, platform-browser, platform-browser-dynamic
... router
Ivy Workspace: Yes

Package                            Version
------------------------------------------------------------
@angular-devkit/architect          0.900.1
@angular-devkit/build-angular      0.900.1
@angular-devkit/build-ng-packagr   0.900.1
@angular-devkit/build-optimizer    0.900.1
@angular-devkit/build-webpack      0.900.1
@angular-devkit/core               9.0.1
@angular-devkit/schematics         9.0.1
@angular/cli                       9.0.1
@ngtools/webpack                   9.0.1
@schematics/angular                9.0.1
@schematics/update                 0.900.1
ng-packagr                         9.0.0
rxjs                               6.5.4
typescript                         3.7.5
webpack                            4.41.2

[rickvandermey]

Does this still work when ivy is disabled?

[PapaNappa]

If I set "enableIvy": false, then the output is '1' as expected and the service is properly injected.

[petebacondarwin]

What happens if you add @Injectable() to the BaseLib1 class?

[petebacondarwin]

OK, so I have dug into the reproduction a bit - thanks for providing this @PapaNappa, it really helps!

I note that in the compiled library we have the following:

/** @nocollapse */
Lib2Service.ctorParameters = () => [
    { type: undefined, decorators: [{ type: Optional }] }
];

Note that the type property is undefined. This means that ngcc has no way of knowing what the type of the optional parameter is, so it cannot generate the correct factory code to attempt to inject this service.

Changing the undefined to BaseLib1 fixes the problem...

[petebacondarwin]

I believe this is an issue with the non-ivy Angular compiler. I created a simple reproduction here: https://github.com/petebacondarwin/angular-issue-35326

[petebacondarwin]

Tracking in FW-1883

[PapaNappa]

Sorry for not responding so quickly.

The same library works with Angular 8, so at least it used to work the non-Ivy compiler producing the wrong ctorParameters.

What I observed is that the original library produces this factory:

Lib2Service.ngInjectableDef = ɵɵdefineInjectable({
  factory: function Lib2Service_Factory() {
    return new Lib2Service(ɵɵinject(BaseLib1, 8));
  },
  token: Lib2Service, providedIn: "root"
});

Notice how BaseLib1 is properly retrieved through ɵɵinject.

ngcc produces the following code/factory:

Lib2Service.ɵfac = function Lib2Service_Factory(t) {
  return new (t || Lib2Service)(ɵngcc0.ɵɵinject(undefined, 8));
};

Notice how this factory now calls ɵɵinject with undefined.

The root cause might thus be, that ngcc extracts the ctor parameter from the (wrong) ctorParameters object. However, it also ignores the original factory function and the parameters it used to pass.

[petebacondarwin]

That is exactly what is happening. Ivy only uses the ɵfac to instantiate the service, whereas ViewEngine uses ngInjectableDef. ngcc only parses the ctorParameters and does not know about ngInjectableDef.

We just need to work out what is generating the invalid undefined and fix it.

[petebacondarwin]

OK so it looks like it is ng-packagr that is actually generating the ctorParameters property and it appears that if the type of a property is not a simple class then it will render undefined.

In this case we have BaseLib1|null which is a union type.

We need to modify the code that ng-packagr uses to handle that scenario. E.g.

angular-cli/packages/ngtools/webpack/src/transformers/ctor-parameters.ts

Lines 139 to 198 in c9a5f3c

	/**
	* Returns an expression representing the (potentially) value part for the given node.
	*
	* This is a partial re-implementation of TypeScript's serializeTypeReferenceNode. This is a
	* workaround for https://github.com/Microsoft/TypeScript/issues/17516 (serializeTypeReferenceNode
	* not being exposed). In practice this implementation is sufficient for Angular's use of type
	* metadata.
	*/
	function typeReferenceToExpression(
	entityNameToExpression: (n: ts.EntityName) => ts.Expression | undefined,
	node: ts.TypeNode,
	typeChecker: ts.TypeChecker,
	): ts.Expression | undefined {
	let kind = node.kind;
	if (ts.isLiteralTypeNode(node)) {
	// Treat literal types like their base type (boolean, string, number).
	kind = node.literal.kind;
	}
	switch (kind) {
	case ts.SyntaxKind.FunctionType:
	case ts.SyntaxKind.ConstructorType:
	return ts.createIdentifier('Function');
	case ts.SyntaxKind.ArrayType:
	case ts.SyntaxKind.TupleType:
	return ts.createIdentifier('Array');
	case ts.SyntaxKind.TypePredicate:
	case ts.SyntaxKind.TrueKeyword:
	case ts.SyntaxKind.FalseKeyword:
	case ts.SyntaxKind.BooleanKeyword:
	return ts.createIdentifier('Boolean');
	case ts.SyntaxKind.StringLiteral:
	case ts.SyntaxKind.StringKeyword:
	return ts.createIdentifier('String');
	case ts.SyntaxKind.ObjectKeyword:
	return ts.createIdentifier('Object');
	case ts.SyntaxKind.NumberKeyword:
	case ts.SyntaxKind.NumericLiteral:
	return ts.createIdentifier('Number');
	case ts.SyntaxKind.TypeReference:
	const typeRef = node as ts.TypeReferenceNode;
	let typeSymbol = typeChecker.getSymbolAtLocation(typeRef.typeName);
	if (typeSymbol && typeSymbol.flags & ts.SymbolFlags.Alias) {
	typeSymbol = typeChecker.getAliasedSymbol(typeSymbol);
	}
	if (!typeSymbol || !(typeSymbol.flags & ts.SymbolFlags.Value)) {
	return undefined;
	}
	const type = typeChecker.getTypeOfSymbolAtLocation(typeSymbol, typeRef);
	if (!type || typeChecker.getSignaturesOfType(type, ts.SignatureKind.Construct).length === 0) {
	return undefined;
	}
	// Ignore any generic types, just return the base type.
	return entityNameToExpression(typeRef.typeName);
	default:
	return undefined;
	}
	}

The code in the ViewEngine compiler that deals with this when computing the ngInjectableDef is here: https://github.com/angular/angular/blob/6ab5f3648a519b3df1a19c9c2fc305a3708202cc/packages/compiler-cli/src/metadata/evaluator.ts#L479-L484

[alan-agius4]

Transferring this the CLI repo since it’s related to tools.