Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Different dependency injection behavior for abstract directives in Ivy #32981

Closed
devversion opened this issue Oct 3, 2019 · 2 comments
Closed

Comments

@devversion
Copy link
Member

devversion commented Oct 3, 2019

It looks like ngtsc behaves differently for abstract directives, compared to the behavior of ngc.

Let's assume we have the following base class in our version 8 application:

class ButtonBase {
  @ViewChild(...) ripple: MatRipple;

  // not using dependency injection.
  constructor(animationMode: string)
}

In version 9 with Ivy, the ButtonClass needs to be decorated with @Directive() because ButtonClass declares a class member with an Angular decorator. This is part of our @angular/core migration for version 9. See migration code.

Adding @Directive() here would be problematic because the ButtonBase class is never intended to be used in combination with dependency injection. i.e. the derived classes manually pass in the constructor parameters through a super(..) call.

The problem here is that ngtsc checks the constructor of ButtonBase now (since we added @Directive) and throws since it cannot find a suitable injection token for type string. The difference is that ngc does not seem to throw here. It seems to skip DI checking for abstract directives. Though ngc throws if some other non-abstract directive/ or component inherits the constructor from ButtonBase. This is reasonable.

No suitable injection token for parameter 'noDiParameter' of class 'BaseDir'. Found string
Here is a patch that can be used to reproduce this behavior in the Angular `compiler-cli` tests.
diff --git a/packages/compiler-cli/test/ngc_spec.ts b/packages/compiler-cli/test/ngc_spec.ts
index 48bac17623..417f6eebda 100644
--- a/packages/compiler-cli/test/ngc_spec.ts
+++ b/packages/compiler-cli/test/ngc_spec.ts
@@ -2287,7 +2287,7 @@ describe('ngc transformer command-line', () => {
     expect(exitCode).toEqual(0);
   });
 
-  describe('base directives', () => {
+  fdescribe('base directives', () => {
     it('should allow directives with no selector that are not in NgModules', () => {
       // first only generate .d.ts / .js / .metadata.json files
       writeConfig(`{
@@ -2297,9 +2297,13 @@ describe('ngc transformer command-line', () => {
       write('main.ts', `
           import {Directive} from '@angular/core';
 
-          @Directive({})
-          export class BaseDir {}
-
+          @Directive()
+          export class BaseDir {
+            // should not check DI parameters here. Derived classes can
+            // manually pass a value to the superclass.
+            constructor(noDiParameter: string) {}
+          }
+  
           @Directive({})
           export abstract class AbstractBaseDir {}
 
@@ -2309,5 +2313,37 @@ describe('ngc transformer command-line', () => {
       let exitCode = main(['-p', path.join(basePath, 'tsconfig.json')], errorSpy);
       expect(exitCode).toEqual(0);
     });
+
+    it('should check di when inheriting constructor from directive without selector', () => {
+      // first only generate .d.ts / .js / .metadata.json files
+      writeConfig(`{
+          "extends": "./tsconfig-base.json",
+          "files": ["main.ts"]
+        }`);
+      write('main.ts', `
+          import {NgModule, Directive} from '@angular/core';
+
+          @Directive()
+          export class BaseDir {
+            constructor(noDiParameter: string) {}
+          }
+  
+          @Directive({selector: 'greet'})
+          export class GreetDir extends BaseDir {}
+
+          @NgModule({declarations: [GreetDir]})
+          export class AppModule {}
+      `);
+
+      // Stub the error spy because we don't want to call through and print the
+      // expected error diagnostic.
+      errorSpy.and.stub();
+
+      const exitCode = main(['-p', path.join(basePath, 'tsconfig.json')], errorSpy);
+      expect(errorSpy).toHaveBeenCalledTimes(1);
+      expect(errorSpy).toHaveBeenCalledWith(
+          jasmine.stringMatching(/Can't resolve all parameters for GreetDir/));
+      expect(exitCode).toEqual(1);
+    });
   });
 });
diff --git a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
index c94c751234..ce0ac89aa6 100644
--- a/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
+++ b/packages/compiler-cli/test/ngtsc/ngtsc_spec.ts
@@ -1022,12 +1022,16 @@ runInEachFileSystem(os => {
         expect(jsContents).toContain('selectors: [["ng-component"]]');
       });
 
-      it('should allow directives with no selector that are not in NgModules', () => {
+      fit('should allow directives with no selector that are not in NgModules', () => {
         env.write('main.ts', `
           import {Directive} from '@angular/core';
 
           @Directive({})
-          export class BaseDir {}
+          export class BaseDir {
+            // should not check DI parameters here. Derived classes can
+            // manually pass a value to the superclass.
+            constructor(noDiParameter: string) {}
+          }
 
           @Directive({})
           export abstract class AbstractBaseDir {}
@@ -1044,6 +1048,27 @@ runInEachFileSystem(os => {
         expect(errors.length).toBe(0);
       });
 
+      fit('should check di when inheriting constructor from directive without selector', () => {
+        env.write('main.ts', `
+          import {NgModule, Directive} from '@angular/core';
+
+          @Directive()
+          export class BaseDir {
+            constructor(noDiParameter: string) {}
+          }
+  
+          @Directive({selector: 'greet'})
+          export class GreetDir extends BaseDir {}
+
+          @NgModule({declarations: [GreetDir]})
+          export class AppModule {}
+        `);
+        const errors = env.driveDiagnostics();
+        expect(errors[0].messageText).toMatch(
+            /No suitable injection token for parameter 'noDiParameter' of class 'BaseDir'/);
+        expect(errors.length).toBe(1);
+      });
+
       it('should not allow directives with no selector that are in NgModules', () => {
         env.write('main.ts', `
           import {Directive, NgModule} from '@angular/core';
@ngbot ngbot bot added this to the needsTriage milestone Oct 3, 2019
@devversion devversion added the hotlist: components team Related to Angular CDK or Angular Material label Oct 3, 2019
@JoostK JoostK self-assigned this Oct 3, 2019
JoostK added a commit to JoostK/angular that referenced this issue Oct 3, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
@ngbot ngbot bot modified the milestones: needsTriage, Backlog Oct 3, 2019
JoostK added a commit to JoostK/angular that referenced this issue Oct 7, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
@alxhub
Copy link
Member

alxhub commented Oct 10, 2019

As @JoostK's commit shows, a fix for this is in progress. Thanks for the report!

alxhub pushed a commit to JoostK/angular that referenced this issue Oct 25, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
alxhub pushed a commit to JoostK/angular that referenced this issue Oct 25, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
alxhub pushed a commit to JoostK/angular that referenced this issue Oct 25, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
alxhub pushed a commit to JoostK/angular that referenced this issue Oct 25, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
crisbeto pushed a commit to crisbeto/angular that referenced this issue Oct 25, 2019
For abstract directives, i.e. directives without a selector, it may
happen that their constructor is called explicitly from a subclass,
hence its parameters are not required to be valid for Angular's DI
purposes. Prior to this commit however, having an abstract directive
with a constructor that has parameters that are not eligible for
Angular's DI would produce a compilation error.

A similar scenario may occur for `@Injectable`s, where an explicit
`use*` definition allows for the constructor to be irrelevant. For
example, the situation where `useFactory` is specified allows for the
constructor to be called explicitly with any value, so its constructor
parameters are not required to be valid. For `@Injectable`s this is
handled by generating a DI factory function that throws.

This commit implements the same solution for abstract directives, such
that a compilation error is avoided while still producing an error at
runtime if the type is instantiated implicitly by Angular's DI
mechanism.

Fixes angular#32981
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Nov 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants