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

[prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor #1056

Open
JounQin opened this issue Oct 8, 2019 · 3 comments

Comments

@JounQin
Copy link

@JounQin JounQin commented Oct 8, 2019

Repro

{
  "rules": {
    "@typescript-eslint/prefer-readonly": 2
  }
}
// your repro code case
class X {
  private _isValid? = true;
  get isValid(): boolean {
    return this._isValid;
  }

  constructor(data?: {}) {
    if (!data) {
      this._isValid = false;
    }
  }
}

Expected Result

No report

Actual Result

@typescript-eslint/prefer-readonly

Member '_isValid' is never reassigned; mark it as `readonly`

Versions

package version
@typescript-eslint/eslint-plugin 2.3.3
@typescript-eslint/parser 2.3.3
TypeScript 3.6.3
ESLint 6.5.1
node 12.11.1
yarn 1.19.0
@JounQin JounQin changed the title [prefer-readonly] set in constructor is ignored ad [prefer-readonly] set in constructor is ignored incorrectly Oct 8, 2019
@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Oct 8, 2019

This isn't actually wrong. _isValid is never reassigned.
I think that you're misunderstanding how classes work in JS.

Assigning a property a default value at declaration time is exactly the same as assigning a default value in the constructor:

class X {
  myVar: number = 1;
}
// is equivalent to
class X {
  myVar: number;
  constructor() {
    this.myVar = 1;
  }
}

This means that your example is actually the same as

class X {
  private _isValid: boolean;

  constructor(data?: {}) {
    this._isValid = true;
    if (!data) {
      this._isValid = false;
    }
  }
}

As far as TS is concerned - a property is not readonly during the constructor.
I.e. your code is correct if you add readonly:

class X {
  private readonly _isValid: boolean = true;
  get isValid(): boolean {
    return this._isValid;
  }

  constructor(data?: {}) {
    if (!data) {
      this._isValid = false;
    }
  }
}

repl

@bradzacher bradzacher closed this Oct 8, 2019
@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Oct 8, 2019

Thanks @bradzacher, the previous seems wrong when I'm using private readonly _isValid = true;, it will emit typeof _isValid === true, so ts will complain on this._isValid = false;. Change it to private readonly _isValid: boolean = true; manually really works, but Should it be handled by eslint-plugin autofixing?

@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Oct 9, 2019

could potentially, but it'll be a really hard fix to implement.
You'll have to detect the type of the value, and print that as a type annotation on the property.

Happy to accept a PR for it. I'll reopen this in case someone wants to implement it.

@bradzacher bradzacher reopened this Oct 9, 2019
@bradzacher bradzacher changed the title [prefer-readonly] set in constructor is ignored incorrectly [prefer-readonly] autofixer doesn't add type to property that is mutated in the constructor Oct 9, 2019
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.