Eslint-plugin-react: False positive with no-unused-state

[viralganatra]

I'm getting a false positive:

[eslint] Unused state field: 'bar' (react/no-unused-state)

when using the following:

import React, { Component, createContext } from 'react';

export const FooContext = createContext();

export default class Foo extends Component {
  constructor(props) {
    super(props);

    this.state = {
      bar: 'baz',
    };
  }

  render() {
    return (
      <FooContext.Provider value={this.state}>
        {this.props.children}
      </FooContext.Provider>
    );
  }
}

Is there a way to bypass this warning? (I'm using the latest version).

The following will bypass it:

<FooContext.Provider value={{ ...this.state }}>

However this will cause unintentional renders for all Context consumers

Thanks

[ljharb]

You should never pass the props or state objects directly. The way to satisfy the warning is to extract the state fields you want, and make a new object with them. If you want to avoid rerenders, don’t pass an object - pass flat props.

[viralganatra]

Interesting, how would you pass flat props to the React context provider given it only accepts the value prop?

https://reactjs.org/docs/context.html#provider

(Is this something that perhaps could be an exception to the rule, or apologies if I'm missing something?)

Thanks

[ljharb]

I'd either use multiple context providers, or i'd make a new object, but memoize it so that the === didn't change unnecessarily.

[viralganatra]

Thanks for the guidance - for me it doesn't make sense to use multiple context providers for things that are related to the same context (in fact I already am using multiple providers). Also Memoizing a new object doesn't really sit right with me just to satisfy this warning.

I think I'll just disable the rule in my file for this case.

Thanks for your help though, appreciate your quick responses 😃

[mdboop]

You should never pass the props or state objects directly. The way to satisfy the warning is to extract the state fields you want, and make a new object with them. If you want to avoid rerenders, don’t pass an object - pass flat props.

What's your reasoning behind this? Passing this.state to a context provider is right in the React docs (here).

import {ThemeContext, themes} from './theme-context';
import ThemeTogglerButton from './theme-toggler-button';

class App extends React.Component {
  constructor(props) {
    super(props);

    this.toggleTheme = () => {
      this.setState(state => ({
        theme:
          state.theme === themes.dark
            ? themes.light
            : themes.dark,
      }));
    };

    // State also contains the updater function so it will
    // be passed down into the context provider
    this.state = {
      theme: themes.light,
      toggleTheme: this.toggleTheme,
    };
  }

  render() {
    // The entire state is passed to the provider
    return (
      <ThemeContext.Provider value={this.state}>
        <Content />
      </ThemeContext.Provider>
    );
  }
}

[ljharb]

The react docs have many examples that have ended up being anti patterns, including putting arrow functions in class fields.

Specifically, exposing a mutable object, for which mutations cause rendering behavior to be undefined, is an unnecessary risk and an unwanted expansion of your component’s public api.. Additionally, destructuring all state and props at the top of every function ensures that, like arguments, the inputs to the function are clearly marked at the top of the scope, ensuring enhanced readability and maintainability.

[jquense]

come on this is bad advice, extracting and constructing a new context object in render is the anti-pattern and causes unnecessary context updates. Forcibly making all context single scalar values is a weird way around this that offers limited improvement while deepening the component tree unnecessarily. Moving the context value to a single state property artificially isn't objectively a better option essentially when the component is only a provider, it's state _is_ the value.

Keeping the rule the way it is is certainly the prerogative of the project and maintainers but claiming that the react docs are actually doing it wrong, and your opinions about readability and what constitutes maintainability is FUD.

[ljharb]

@jquense I'm not sure you're understanding the thread - while I do see how passing a new object created every time in render will break PureComponent-like optimizations, that doesn't change the underlying best practice - not FUD - that unintentionally passing around objects is expanding the API of your component.

There's nothing wrong with anyone claiming anyone is doing it wrong - that they're "the React docs" don't mean they're always doing things the best way.

[callmenick]

A discussion on best practices doesn't really add anything to the original concern though, and that's that @viralganatra is seeing a false positive whereby he's using all the state, but the rule is telling him he isn't.

As @jquense said though, it's up to you folks to write the rules and such, but I think the discussion derailed pretty quickly.

[ljharb]

@callmenick indeed, but the limits of static analysis are that passing around the entire state, props, or context object makes it impossible to detect which properties are used. Since this is a bad practice anyways, though, there's not really much value in trying to address it.

[nurbek-ab]

@ljharb In certain scenarios passing the entire state is necessary. Consider the following case:
a react form with a lot of fields has a container component that fetches data, populates its state with that data and passes the state to the functional child component. I don't really want to destruture the whole state and pass each field one by one just to satisfy ESLint in this particular case. I just want to do something like this:

  render() {
    const { isSubmitting, errors, ...fields } = this.state;
    return (
      <Form
        fields={fields}
        isSubmitting={isSubmitting}
        onSubmit={this.onSubmit}
        onInputChange={this.onInputChange}
        errors={errors}
      />
    );
  }

So in this case if ESLint just assumes that the whole state is used becaused it passed as a prop is enough. Assuming that none of the fields is used later is wrong IMHO.

[ljharb]

You shouldn’t do it to satisfy eslint, you should do it because then someone reading this component doesn’t have to read a different component to figure out what your component’s API really is.

Always explicitly destructure everything and re-provide it.

[nurbek-ab]

@ljharb If by API you mean a set of component's props, then it can b easily described using prop-types. No need to pass each field separatelly just to show the API:

Form.propTypes = {
  fields: T.shape({
    first_name: T.string,
    last_name: T.string,
    email: T.string
  }).isRequired
}

[ljharb]

So you’re ok with duplicating the field name in both components’ propTypes, but not in the props themselves?

[nurbek-ab]

In the container component the fields are state fields not props. So no duplicating in this case.

[ljharb]

What i mean then is, where you render Form, it should be explicit what props you’re passing without having to look at Form itself.

[Lliuxs]

i use antd config and react docs about context have a error Unused state field: 'toggleTheme'eslint(react/no-unused-state)
how could i fix this

[ljharb]

@Lliuxs never pass the state object directly. Explicitly destructure the properties you need out of the state, and pass a new object to the context provider.

[djErock]

In the documentation's example constructor -->

constructor(props) {
    super(props);

    this.toggleTheme = () => { // remove this and the lint error shows up
      this.setState(state => ({
        theme:
          state.theme === themes.dark
            ? themes.light
            : themes.dark,
      }));
    };

    // State also contains the updater function so it will
    // be passed down into the context provider
    this.state = {
      theme: themes.light,
      toggleTheme: this.toggleTheme,
    };
  }

They are essentially using the prop in the toggle function which removes this lint error. This is a bit confusing as when you are attempting to alter the docs example for your own and remove that toggle function the red squiggly reappears. I would love to see someone from the react team chime in here... Just wanted to add this, I will go back to my popcorn now and watch the comments...