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-unused-state using functional setState #1484

Closed
cristianfraser opened this issue Oct 17, 2017 · 19 comments
Closed

no-unused-state using functional setState #1484

cristianfraser opened this issue Oct 17, 2017 · 19 comments

Comments

@cristianfraser
Copy link

This code:

class MyComponent extends Component {
  state = {
    foo: true,  <---- here eslint shows error
    bar: [1, 2, 3],
  }

  fooBar = () => {
    this.setState(prevState => ({
      bar: prevState.foo ? prevState.bar.sort() : prevState.bar.reverse(),
      foo: !prevState.foo,
    }))
  }

  render() {
    return (<div onClick={this.fooBar}>{...this.state.bar}</div>)
  }
}

But eslint shows error for foo, no-unused-state is displayed, but it is being used.

@ljharb
Copy link
Member

ljharb commented Oct 17, 2017

Can you share the error, and your eslint config?

@neiker
Copy link

neiker commented Oct 17, 2017

Same here.

onTitleChange = (title) => {
    this.setState({ title, titleChanged: true });
  }

  onTemplateChange = (template) => {
    this.props.navigator.pop();

    this.setState((state => ({
      template,
      title: state.titleChanged ? state.title : getTemplateTitle(template),
    })));
  };

97:28 error Unused state field: 'titleChanged' react/no-unused-state

I'm using eslint-config-airbnb

@ljharb
Copy link
Member

ljharb commented Oct 17, 2017

Your eslint config must be different, because the airbnb config does not permit class properties to be used.

@cristianfraser
Copy link
Author

I'm also using eslint-config-airbnb and get the same error

@ljharb
Copy link
Member

ljharb commented Oct 17, 2017

@cristianfraser same question; can you share your code and your eslint config?

Again, if you're using class properties, you're violating the airbnb style guide, so if you are, you must be using a modified eslint config.

@neiker
Copy link

neiker commented Oct 17, 2017

{
    "parser": "babel-eslint",
    "extends": [
        "airbnb"
    ],
    "env": {
        "mocha": true
    },
    "rules": {
        "no-restricted-properties": "off",
        "complexity": [ "error", 10 ],
        "max-depth": [ "error", 4 ],
        "max-nested-callbacks": [ "error", 2 ],
        "max-params": [ "error", 4 ],
        "max-statements": [ "error", 18 ],
        "no-constant-condition": "off",
        "no-plusplus": "off",

        "no-warning-comments": "error",
        "comma-dangle": [
            "error", 
                {
                "arrays": "always-multiline",
                "objects": "always-multiline",
                "imports": "always-multiline",
                "exports": "always-multiline",
                "functions": "always-multiline"
            }
        ],
        "no-restricted-globals": "off",

        "react/require-default-props": "off",
        "react/no-unused-prop-types": "off",
        "react/jsx-filename-extension": [
            "warn",
            {
                "extensions": [".js"]
            }
        ],
        "react/jsx-no-bind": [
            "error",
            {
                "ignoreRefs": false,
                "allowArrowFunctions": false,
                "allowBind": false
            }
        ],
        "react/no-typos": "off",

        "import/no-named-as-default": "off",
        "import/prefer-default-export": "off",

        "jsx-a11y/href-no-hash": "off",
        "jsx-a11y/anchor-is-valid": "off",

        "react-native/split-platform-components": "off",
        "react-native/no-unused-styles": "error",
        "react-native/no-inline-styles": "error",
        "react-native/no-color-literals": "error"
    },
    "globals": {
        "__DEV__": true,
        "fetch": true,
        "ErrorUtils": true
    },
    "plugins": [
        "react-native"
    ]
}

@cristianfraser
Copy link
Author

cristianfraser commented Oct 17, 2017

This is a minimal code that shows no-unused-state for ascending:

import React from 'react'


export default class TestComponent extends React.Component {

  state = {
    list: [1, 2, 3],
    ascending: true,
  }

  onClick = () => {
    this.setState(({ list, ascending }) => {
      if (ascending) {
        return {
          list: list.sort(),
          ascending: !ascending,
        }
      }

      return {
        list: list.reverse(),
        ascending: !ascending,
      }
    })
  }

  render() {
    return (
      <div>
        <button onClick={this.onClick}>Click me</button>
        {this.state.list.map(item => <div>{item}</div>)}
      </div>
    )
  }
}

This is my eslint config:

{
  "extends": "airbnb",
  "parser": "babel-eslint",
  "env": {
    "browser": true,
    "node": true,
    "es6": true,
    "jest": true
  },
  "plugins": [
    "react"
  ],
  "settings": {
    "import/resolver": {
      "webpack": { "config": "webpack/dev.config.js" }
    }
  },
  "rules": {
    "camelcase": 0,
    "semi": [2, "never"],
    "react/require-default-props": 0,
    "react/jsx-filename-extension": 0,
    "jsx-a11y/label-has-for": [2, {
      "components": ["label"],
      "required": { "some": ["nesting", "id"] }
    }],
    "jsx-a11y/click-events-have-key-events": 0,
    "jsx-a11y/anchor-is-valid": ["error", {
      "components": ["Link"],
      "specialLink": ["to"],
      "aspects": ["noHref", "invalidHref", "preferButton"]
    }],
  }
}

Also, I've used class properties with plain config-airbnb with no problems.

@ljharb
Copy link
Member

ljharb commented Oct 18, 2017

@cristianfraser you can only use it with babel-eslint; the airbnb config uses the default parser. It's likely the react-native plugin is enabling that as well, of course.

@cristianfraser
Copy link
Author

What do you mean by I can only use it with babel-eslint? That's what's in my config, that's the wrong parser for that rule? Is what's happening the expected behaviour in this case?

@ljharb
Copy link
Member

ljharb commented Oct 18, 2017

@cristianfraser eslint core doesn't support parsing class properties; only babel-eslint does - thus if you just had extends: "airbnb" you wouldn't be able to use them.

What's happening seems like a bug in the no-unused-state rule, I'm just trying to gather as much info about it as possible :-)

cc @wbinnssmith @jackyho112

@jackyho112
Copy link
Contributor

jackyho112 commented Oct 18, 2017

@ljharb Thanks for tagging me! This rule doesn't support arrow function definition as class method currently. I think this will add it.

@ljharb
Copy link
Member

ljharb commented Oct 18, 2017

Fixed with #1411

@ljharb ljharb closed this as completed Oct 18, 2017
@Godsenal
Copy link

I'm having a same problem with latest verstion.

state = {
    removeQueue: [],  <--- unused state field: 'removeQueue'
}
onUpdate = (date, id) => {
    this.setState(prevState => ({
      removeQueue: [...prevState.removeQueue, { date, id }],
    }));
  }

I'm using removeQueue in functional setState, but eslint shows error.

@ljharb
Copy link
Member

ljharb commented Feb 19, 2018

Try making onUpdate a constructor-bound instance method (putting arrow functions in class properties is a bad practice) and it should pick it up.

@Godsenal
Copy link

constructor() {
    super();
    this.onUpdate = this.onUpdate.bind(this);
  }

onUpdate(date, id) {
    this.setState(prevState => ({
      removeQueue: [...prevState.removeQueue, { date, id }],
    }));
  }

I've changed it like this.

But it shows me the same error...

@ljharb
Copy link
Member

ljharb commented Feb 19, 2018

Thanks - could you file a new issue with both code examples (the class field and the instance method)?

@Godsenal
Copy link

sure!
#1697

@mytharcher
Copy link

I meet the same error. Here is my minimal code:

import React from 'react';

class Test extends React.Component {
  static Sub({ state }) {
    const { property } = state;
    return <p>{property}</p>;
  }

  state = {
    property: 1 // Unused state field 'property'
  }

  render() {
    return <Test.Sub state={this.state} />;
  }
}

export default Test;

And it is same when I use constructor style.

I am using the latest eslint and etc.

@ljharb
Copy link
Member

ljharb commented Dec 18, 2018

@mytharcher it’s an antipattern to pass around the entire props or state objects; and it basically prevents any form of static analysis on it. Destructure it first, and explicitly pass the members you need.

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

No branches or pull requests

6 participants