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

[no-useless-constructor] `constructor` with `super(someArgs)` is not useless in `Angular` #1164

Closed
JounQin opened this issue Oct 31, 2019 · 7 comments

Comments

@JounQin
Copy link

@JounQin JounQin commented Oct 31, 2019

Repro

{
  "rules": {
    "@typescript-eslint/no-useless-constructor": 2
  }
}
class A {
  protected http: HttpClient
  constructor(protected injector: Injector) {
    this.http = this.injector.get(HttpClient)
  }
}

class B implements A {
  // this should not be considered as useless due to DI
  constructor(injector: Injector) {
    super(injector)
  }
}

Expected Result

No error report

Actual Result

Error

Versions

package version
@typescript-eslint/eslint-plugin 2.6.0
@typescript-eslint/parser 2.6.0
TypeScript 3.6.4
ESLint 6.6.0
node 13.0.1
npm 6.12.0
@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Oct 31, 2019

We can use following workaround temporarily:

class B implments A {
  constructor(protected injector: Injector) {
    super(injector);
  }
}

But if A declare it as private, we can only change it to another property name like:

class A {
  private http: HttpClient
  constructor(private injector: Injector) {
    this.http = this.injector.get(HttpClient)
  }
}

class B implements A {
  constructor(private _injector: Injector) {
    super(_injector)
  }
}
@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Oct 31, 2019

Why do you keep writing class B implements A?
That's not valid code?


Could you please explain why you need the constructor on the child class?

Unless I'm mistaken, this is perfectly valid code, which does exactly the same thing, without the useless constructor:

class A {
  protected http: HttpClient
  constructor(protected injector: Injector) {
    this.http = this.injector.get(HttpClient)
  }
}

class B extends A { }
@bradzacher bradzacher added awaiting response and removed triage labels Oct 31, 2019
@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Oct 31, 2019

Oh sorry, it should be extends, it's a typo.

B's constructor is required in favor of DI system in Angular.

@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Nov 1, 2019

Have you got some docs on why the constructor is explicitly needed on the child?
I haven't used angular since like 1.6, so I don't know how it works.

Unless I'm mistaken it shouldn't need it explicitly, as the child should implicitly inherit the parent's constructor.

@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Nov 1, 2019

@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Nov 1, 2019

Please be careful with your @ tagging. You tagged the wrong account.
Github has a crappy implementation, and doesn't put my name first in the autocomplete list.


Looking into this, I have no idea why it's required. I think angular statically analyses the file by itself? I'm not sure, the custom compiler is a black box to me.

We don't want to encode the semantics of angular into typescript.
The fact that the constructor is required by the angular compiler is an implementation detail of the framework.

If you'd like this functionality, I'd suggest getting a rule created in a plugin like eslint-plugin-angular.

@bradzacher bradzacher closed this Nov 1, 2019
@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Nov 1, 2019

@bradzacher Thx for your patients. I will open a feature request issue at angular-eslint then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.