Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork2.8k
Description
Before You File a Proposal Please Confirm You Have Done The Following...
- I havesearched for related issues and found none that match my proposal.
- I have searched thecurrent rule list and found no rules that match my proposal.
- I haveread the FAQ and my problem is not listed.
My proposal is suitable for this project
- I believe my proposal would be useful to the broader TypeScript community (meaning it is not a niche proposal).
Link to the rule's documentation
https://typescript-eslint.io/rules/parameter-properties/
Description
TL;DR
This issue proposes the following changes to the ruleparameter-properties
:
- [Primary] Additionally check the error-prone usage of the feature
parameter-property
under the class inheritance:classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{// vvvvvv Error: Unnecessary TS visibility modifier for parameter property since the variable `a` is initialized by its parent classconstructor(publica:string,publicb:number){super(a);}}
- [Optional] Merge the check by the rule
no-unnecessary-parameter-property-assignment
to this rule.
Background
Currently, the ruleparameter-properties
strictly reports the following TS parameter property usage whenOptions.prefer
is set toparameter-property
and conditions marked in comments are met:
classFoo{// vvv Any TS visibility modifierpublica:string;// v No TS visibility modifierconstructor(a:string){// ^ ^^^^^^ The same variable name and type as defined in line 2this.a=a;// ^^^^^^^ The assignment expression AT THE BEGINNING of the constructor}}
which expects it to be refactored as the following (Though no auto-fix was provided) topreferparameter-property
:
classFoo{constructor(publica:string){/* Empty */}}
However, the TS featureparameter-property
comes with an error-prone usage (duplication):
constructor(publica: string){this.a=a;// Unnecessary assignment expression since TS will generate this line while compiling to JS}
A developer would not intentionally write code like this, so the ruleno-unnecessary-parameter-property-assignment
detects it.
Motivation
During our feature usage analysis, we found another error-prone usage involving thesuper
call (that is, the class inheritance):
classFoo{// vvvvvv This makes parameter `a` a property of class `Foo`constructor(publica:string){/* Empty */}}classBarextendsFoo{// vvvvvv This also makes parameter `a` a property of class `Bar`constructor(publica:string,publicb:number){super(a);}}
causing the assignment expressionthis.a = a
appears twice in classFoo
and classBar
respectively (See JS code inthis playground).
This is not a niche proposal since a real-world case can be found in a famous repositoryBabylonJS/Babylon.js
, where
defaultViewer.ts
declares a classDefaultViewer
that extends from a parent classAbstractViewerWithTemplate
.- The ultimate parent class of
DefaultViewer
can be traced back to classAbstractViewer
inviewer.ts
. - The parent class
AbstractViewer
declares a parameter property withpublic containerElement: Element
. - The child class
DefaultViewer
also DECLARES a parameter property withpublic containerElement: Element
and sends it to thesuper()
call. - As demonstrated previously, this duplicates the assignment expression, causing similar problems that the rule
no-unnecessary-parameter-property-assignment
detects.
This case is considered a bad practice by us because, on the contrary, we also found the following best practice as demonstrated by the repositoryglideapps/quicktype
, where
Dart.ts
classDartRenderer
-extends->ConvenienceRenderer.ts
classConvenienceRenderer
-extends->Renderer.ts
classRenderer
.- The first constructor parameter
targetLanguage
is markedprotected readonly
in the parent class and not marked in the following derived classes. - This only initializes the class property once at the parent class, preventing it from accidentally undoing the parent's initialization.
Proposal
Given the motivation described above, we would like to (mainly) propose the enhancement of the unnecessary-check under the class inheritance:
- Ideally, a cross-file inheritance chain analysis is desired, given the proposed scenario may involve two classes in different files.
- A loose constraint may simplify the implementation in case the precise cross-file type analysis is currently not available or time-consuming:
- In a derived class's constructor, a parameter is marked with TS visibility modifier.
- That parameter is sent to the
super
call.
This check benefits a better use of the featureparameter-property
.
Optional Goal
Like the ruleno-unnecessary-parameter-property-assignment
, a developer would not intentionally write duplicated code once they determined toprefer: 'parameter-property'
. We consider violations of best practices like these should be implemented seamlessly and automatically without another rule to turn on.
The base ruleparameter-properties
was discussed in2019 and implemented in2022, and the ruleno-unnecessary-parameter-property-assignment
was discussed in2023 and introduced later in2024. We have read all discussions in PRs and issues and found that merging them into one rule was not discussed previously. By the chance of enhancing the base rule, we would like to pose the discussion for a more concise and compact rule set.
Fail
classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{// vvvvvv Error: Unnecessary modifier, has already been initialized in the parent class.constructor(publica:string,publicb:string){super(a);}}
Pass
classFoo{constructor(publica:string){/* Empty */}}classBarextendsFoo{constructor(a:string,publicb:string){// v ^ If the parameter is passed to the `super` call, it would most likely unnecessary to be a parameter property.super(a);}}
Additional Info
Current rules can not detect the bad practice discussed in this proposal.