no-access-state-in-setstate false negative in async class method

[armordecai]

I'm getting a false positive from the no-access-state-in-setstate rule with this method in a class component:

async onSubmit(event) {
    event.preventDefault()
    this.setState({ submitting: true })

    const res = await fetchJSON(
        "/api/user",
        "POST",
        this.state.values, // error: "Use callback in setState when referencing the previous state."
    )

    if (!res.error) {
        return this.props.router.push("/")
    }

    this.setState({
        submissionError: res.error,
        submitting: false,
    })
}

If I rewrite that as fetchJSON(...).then(res => ...) the error goes away. (It actually goes away if I just remove the const res = part, but then I can't use the returned value.)

• eslint 7.24.0
• eslint-plugin-react 7.23.2

Here's my ESLint configuration:

module.exports = {
    env: {
        browser: true,
        commonjs: true,
        es2021: true,
        node: true,
    },
    extends: [
        "eslint:recommended",
        "plugin:react/recommended",
    ],
    parserOptions: {
        ecmaVersion: 2021,
        sourceType: "module",
        ecmaFeatures: {
            jsx: true,
        },
    },
    plugins: [
        "react",
    ],
    rules: {
        // ...
        "react/button-has-type": 2,
        "react/jsx-child-element-spacing": 2,
        "react/jsx-curly-brace-presence": [1, "never"],
        "react/jsx-curly-newline": [1, "consistent"],
        "react/jsx-curly-spacing": [1, { when: "never", allowMultline: true }],
        "react/jsx-equals-spacing": [1, "never"],
        "react/jsx-first-prop-new-line": [1, "multiline-multiprop"],
        "react/jsx-indent": [1, "tab"],
        "react/jsx-no-script-url": 2,
        "react/jsx-no-target-blank": 1,
        "react/jsx-no-useless-fragment": 1,
        "react/jsx-props-no-multi-spaces": 1,
        "react/jsx-tag-spacing": [1, { beforeClosing: "never", beforeSelfClosing: "always" }],
        "react/jsx-wrap-multilines": 1,
        "react/no-access-state-in-setstate": 2,
        "react/no-adjacent-inline-elements": 2,
        "react/no-unused-prop-types": 2,
        "react/no-unused-state": 2,
        "react/prop-types": 0,
        "react/self-closing-comp": 1,
        "react/state-in-constructor": 2,
    },
    settings: {
        react: {
            version: "detect",
        },
    },
}

[Wesitos]

I was able to reproduce the issue with this code

class Foo extends React.Component {
  onSubmit() {
    this.setState({submitting: true});

    const result = this.state.values;

    this.setState({
      submitting: false,
      result
    });
  }
}

[Wesitos]

Also this one

class Foo extends React.Component {
  foo() {
    this.setState({
      submitting: false,
      result: this.state.values
    });
  }
}

[ljharb]

@Wesitos where's the false positive? While in both of those examples, nothing calls or references those class methods (which makes them dead code and thus, they should be deleted), they're still likely incorrect.

[Wesitos]

@ljharb My first approach is to reduce the code. Both code snippets give me a no-access-state-in-setstate (with version 7.24.0) . I believe these snippets are equivalent to the one of @armordecai. I was not sure if these were false positives or expected behavior. Do you consider @armordecai 's issue (or his workaround) is expected behavior?

[ljharb]

Actually, after rereading the OP, I don't think there's a bug here at all.

The code is supposed to look something like this (i'm using new Promise here so the later awaits can be used):

async onSubmit(event) {
    event.preventDefault()
        const { values } = new Promise((resolve) => {
        this.setState({ submitting: true }, resolve);
    });

    const res = await fetchJSON(
        "/api/user",
        "POST",
        values,
    );

    if (!res.error) {
        return this.props.router.push('/');
    }

    return new Promise((resolve) => {
        this.setState({
            submissionError: res.error,
            submitting: false,
        }, () => resolve());
    });
}

Your first minimal example is a correct warning.

Your second minimal example, however, is a bug - since you're accessing this.state before the setState call happens, not after.

I'm going to close this as "working as intended", but please file that second example as a separate issue so it can be fixed.

[armordecai]

Actually, after rereading the OP, I don't think there's a bug here at all.

@ljharb I disagree. The two examples @Wesitos posted are not false positives -- both (effectively) access this.state inside a this.setState call, which is what the no-access-state-in-setstate rule should be warning about. By contrast, the example in my OP does not pass anything derived from this.state to any setState call, so the rule should have no opinion about it.

The code is supposed to look something like this (i'm using new Promise here so the later awaits can be used):

JavaScript doesn't require that an async function returns a promise. If some people would like to add that requirement that as a matter of preference, that could be a candidate for a new ESLint rule, but it doesn't have anything to do with this rule, or even with React in general. In my case, this function is an event handler, so any value it returns, promise or otherwise, will never be used, or even seen, by anything.

As for the first part of your suggested rewrite, I guess you were concerned about my fetchJSON call potentially happening before the re-render from changing the submitting state to true was finished, but that state only controls a visual UI affordance (letting users know that clicking the button actually did something), and there's no reason for that to block or delay sending the network request. Even if the network request fully completes before the first re-render, that still doesn't matter, since in most cases the component will be unmounted anyway at that point, and even in the error case, React handles multiple setState calls in the order they were made, so the "worst case" scenario is that the first re-render doesn't actually happen, and the submitting state never actually changes, but that wouldn't be a problem at all.

And keep in mind, even if we have different opinions about the best / safest / prettiest way to write this, the code in my OP works flawlessly and is perfectly valid JavaScript, and doesn't logically violate this specific rule, as there is no state access in any setState call.

[ljharb]

@armordecai once setState has been invoked, it is no longer safe to access this.state.

If in your specific use case, you don't care about the risk there, an eslint override is appropriate - but the guidance of the rule, and the React team itself, is that such access must be done inside the callback, or before the setState call.

[armordecai]

@ljharb Even so, I think there's still a bug here, you may just want to solve it in the opposite direction. As mentioned in the OP, if I rewrite this:

const res = await fetchJSON(..., this.state.values)
setState({ ..., submissionError: res.error })

as this:

fetchJSON(..., this.state.values).then(res => {
   setState({ ..., submissionError: res.error })
})

then the rule passes, even though the difference is only syntactic sugar. I don't think either one should be considered a rule violation (the state values I'm accessing after the setState call aren't in any way affected by the setState call, and React doesn't temporarily remove or alter the rest of the state while an update is in progress), but if one is, then both should be.

[ljharb]

I agree, both should be errors. setState calls can be batched, so you can’t actually guarantee that it’s not affected by the setState call.

[armordecai]

setState calls can be batched, so you can’t actually guarantee that it’s not affected by the setState call.

Maybe I'm missing something, but none of the setState calls in my code include values, so that seems like a pretty solid guarantee that this.state.values won't be affected, whether or not the setState calls are batched?

[ljharb]

I realize that in your specific case there's no risk. However, setState calls could be batched from anywhere in the component - not just this method - and in dynamic ways, and the limits of static analysis are such that it's simply not a safe thing to allow.

A parallel would be == - when you're comparing things that can only ever be of the same type, you can make an argument that it's "safe" to use ==. But, the linter rule (and styleguides) will tell you to always use === just in case - because it's a safer habit to get into.

The same applies here. It is a much safer habit to get into to write your code safely than to use an unsafe pattern that one-off specifics happen to make safe.

[joset98]

I was able to reproduce the issue with this code

and there is where i am using