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

sort-comp: 'instance-variables' doesn't work properly #1664

Closed
m10 opened this issue Jan 29, 2018 · 14 comments
Closed

sort-comp: 'instance-variables' doesn't work properly #1664

m10 opened this issue Jan 29, 2018 · 14 comments
Labels

Comments

@m10
Copy link

m10 commented Jan 29, 2018

When enabling sort-comp "react/sort-comp": 1 with [email protected]
i would expect that instance properties should be placed at the beginning of the class definition.
But with code like

class MyComp extends Component {
  myProp = null;
  state = {...}

  componentWillReceiveProps(props) {...}
  render() {...}
}

i get the following warning:

myProp should be placed after componentWillReceiveProps  react/sort-comp

ref: #1587

@ljharb
Copy link
Member

ljharb commented Jan 29, 2018

Did you specify an ordering? The default ordering doesn’t include instance methods or instance variables.

@m10
Copy link
Author

m10 commented Jan 29, 2018

no i did not specify any custom ordering. I'd expect that the sorting order of custom properties is at least ignored (if not enforced to the top) by default

@ljharb
Copy link
Member

ljharb commented Jan 29, 2018

Anything that's not explicitly specified is grouped under "everything-else", per the docs.

@m10
Copy link
Author

m10 commented Jan 29, 2018

Yes, I understood that. But it is a weird default that instance properties should be placed somewhere in the middle of the class body

@ljharb
Copy link
Member

ljharb commented Jan 29, 2018

They’re not yet part of the language, and when the rule was created, they didn’t exist yet :-)

@m10
Copy link
Author

m10 commented Jan 29, 2018

well, then it should be changed now that they exist?

@ljharb
Copy link
Member

ljharb commented Jan 29, 2018

That would be a breaking change; next time we do one, we can change that default. For now, you’ll need to manually specify your ordering.

@m10
Copy link
Author

m10 commented Jan 29, 2018

alright! Thanks for the feedback!

@sekoyo
Copy link

sekoyo commented Mar 20, 2018

These should really go at the top without generating an eslint error, they shouldn't be bundled into "everything else"

They are stage 3 and not experimental https://github.com/tc39/proposal-class-fields

@ljharb
Copy link
Member

ljharb commented Mar 20, 2018

Stage 3 is still experimental; only stage 4 is final.

Again, you can add this category yourself to your own config - and in the next major, it’ll be a default.

@serhiipalash
Copy link

serhiipalash commented Feb 4, 2019

Hi @ljharb! Is it possible to make instance-variables work with [email protected] (latest at the moment)?

Here is our rule config

{
  "rules": {
    "react/sort-comp": [
      "warn",
      {
        "order": [
          "static",
          "static-methods",
          "state",
          "instance-variables",
          "/^constructor/",
          "render",
          "lifecycle",
          "/^_.+$/",
          "everything-else"
        ],
        "groups": {
          "lifecycle": [
            "componentWillMount",
            "componentDidMount",
            "componentWillReceiveProps",
            "shouldComponentUpdate",
            "componentWillUpdate",
            "componentDidUpdate",
            "componentWillUnmount"
          ]
        }
      }
    ]
  }
}

but it seems like instance-variables doesn't have any affect. All instance variables are warned and asked to be placed at the bottom of class.

@serhiipalash
Copy link

Somehow everything works fine, when I run eslint in Terminal, but in Sublime instance-variables are highlighted as warnings, if they are at the top.

@ljharb
Copy link
Member

ljharb commented Feb 4, 2019

@serhiipalash that implies that sublime isn't using your local eslint - if you have a global one installed, try removing it.

@serhiipalash
Copy link

@ljharb actually the problem was with create-react-app. It goes with its own ESLint config and installs eslint package with different from latest version.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

No branches or pull requests

4 participants