Linkerd2: Enable the react/no-did-update-set-state eslint rule

[klingerf]

In #3882, we adopted more eslint rules for our Javascript codebase. The react/no-did-update-set-state rule is recommended, but it wasn't enabled as part of that PR, since it requires code changes. We should enable the rule and fix the code that fails to lint. We can enable it by removing the override that appears here:

https://github.com/linkerd/linkerd2/blob/0eabbe4173fd8a7da91d5771572bd08d85268275/web/app/.eslintrc#L85-L88

[christyjacob4]

Hi @klingerf I'd like to take up this issue. Could you please assign it to me

[klingerf]

@christyjacob4 Great, thanks! Have assigned it to you.

[christyjacob4]

@klingerf thank you

Going through the getting started guide, I understand that there are two configurations to develop linkerd, namely Comprehensive and Web.

If I'm not wrong, for the scope of this issue, I would only be dealing with the Web Configuration?

[klingerf]

@christyjacob4 Yep, that's exactly right. I recommend following the instructions here:

https://github.com/linkerd/linkerd2/blob/master/BUILD.md#web

[christyjacob4]

Hey @klingerf I followed the instructions in the BUILD.md file but had a tough time getting the Configuration Setup to work. (PS it's still not working) .
I moved on to the web build. It was giving me the following error.

Anyways, Here are a few questions

1. Do I require all this to test the eslint part?
2. What is the command to perform the linting.

[klingerf]

Hey @christyjacob4, linting happens automatically as part of running webpack, and you can run webpack locally by running bin/web build (instead of bin/web dev).

But you will require a working linkerd install to test your changes. There are a lot of ways to go about that. It's probably easiest to jump into the #contributors channel on the linkerd slack (slack.linkerd.io).

[christyjacob4]

Thanks for that @klingerf . I will reach out on slack for additional information.

[christyjacob4]

Hi @klingerf Does this count as a valid fix? This doesn't result in any warning in bin/web build

componentDidUpdate(prevProps) {
    const { match, isPageVisible } = this.props;
    const { params } = match;
    if (!_isEqual(prevProps.match.params.namespace, params.namespace)) {
      // React won't unmount this component when switching resource pages so we need to clear state
      this.resetState(params);
    }

    handlePageVisibility({
      prevVisibilityState: prevProps.isPageVisible,
      currentVisibilityState: isPageVisible,
      onVisible: () => this.startServerPolling(),
      onHidden: () => this.stopServerPolling(),
    });
  }

resetState(params) {
    this.api.cancelCurrentRequests();
    this.setState(this.getInitialState(params));
  }

Please correct me if I'm wrong.

[cpretzer]

@christyjacob4 in which file are you making this change?

The best way to get feedback about code is to submit a PR. That way folks can test changes in their own environments by checking out your branch.

[christyjacob4]

@cpretzer Im making these changes in web/app/js/components/Namespace.jsx
The task is to refactor code that fails to lint, on enabling the react/no-did-update-set-state eslint rule.
I haven't raised a PR because the task isn't complete.
I want to know if what I did, is the right way of doing it.

[christyjacob4]

@klingerf @cpretzer from the react documentation, they suggest to wrap it in a condition. They don't say that it is wrong
https://reactjs.org/docs/react-component.html#componentdidupdate

I also think that we must remove the rule from .eslintrc as discussed by people here
https://github.com/yannickcr/eslint-plugin-react/issues/1707

[cpretzer]

that's really good info @christyjacob4

As I read it, no-did-update-set-state will be modified to check for the condition that you mentioned: https://github.com/yannickcr/eslint-plugin-react/issues/1707#issuecomment-521826558

It's unclear as to whether that has been added yet, so if your PR addresses the eslint error message, I'm happy with that until the eslint logic is updated.

WDYT @klingerf ?