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

[member-ordering] public member reference private member should be ignored #1166

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

[member-ordering] public member reference private member should be ignored #1166

JounQin opened this issue Oct 31, 2019 · 2 comments

Comments

@JounQin
Copy link

@JounQin JounQin commented Oct 31, 2019

Repro

{
  "rules": {
    "@typescript-eslint/member-ordering": 2
  }
}
export class A {
  private a: number
  private b: number

  c = this.a + this.b
}

Expected Result

Do not report error

Actual Result

Unfixable

Additional Info

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 12.13.0
npm 6.12.0
@bradzacher

This comment has been minimized.

Copy link
Member

@bradzacher bradzacher commented Oct 31, 2019

same as #417

Detecting the dependencies between fields is trivial, but using that to inform how the sort works is very complex.

For example:

export class A {
  private a: number
  private b: number
  private d: number

  protected e: number

  c = this.a + this.b
}

where does c go?

  • does it go directly below a and b, which is the highest available place which also satisfies the dependency constraints?
  • does it go after d, which is the highest available place after a block (i.e. doesn't interrupt the private modifier group) that also satisfies the dependency constraint?
  • does it go at the end of all "field" type members (i.e. after e), so that it's a lonely public field?
  • does it get ignored completely, and we leave it up to the dev?

What about if you introduce another constraint:

export class A {
  private a: number
  private b: number
  private d: number

  protected e: number

  c = this.a + this.b
  private f = this.c
}

now what happens to f? where does it go?

It's not always easy to detect when there is a dependency as well - there are a lot of cases that are irrelevant or just hard to detect via the AST.

abstract class Foo {
    a = 1;
   
    abstract boo: number;
    get aaa() { return this.boo }
}
class Bar extends Foo {
    // irrelevant dependency on parent field
    b = this.a;
    
    // irrelevant dependency on a class method
    c = this.myMethod;
    myMethod() {}
    
    get foo() { return this.b }
    // hard to detect dependency on this.b
    bar = this.foo

    boo = 1;
    // impossible to detect dependency on this.boo
    baz = this.aaa
}

Dependency based ordering constraints add a lot of complexity into an already complex rule (which is also getting more complex in #263).


The suggestion is to either use a disable comment, or use a constructor to completely avoid this problem:

export class A {
  c: number;

  private a: number
  private b: number
  private d: number
  private f: number

  protected e: number

  constructor() {
    this.c = this.a + this.b
    this.f = this.c
  }
}
@bradzacher bradzacher closed this Oct 31, 2019
@JounQin

This comment has been minimized.

Copy link
Author

@JounQin JounQin commented Nov 1, 2019

@bradzacher Why can't we just add an option to ignore this case or by default?

It is something just related to code style, I don't think it's worth to change my source code into constructor only due to this.

a disable comment is a bad idea if we have a lot of public member relied on private member(s).

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.