Refactor willReceiveProps usages into getDerivedStateFromProps

[tstirrat15]

Motivation

Pull #128 fixed the warnings around willReceiveProps, but it did so by essentially silencing them. Given that willReceiveProps is deprecated, it'd be better to move to a different approach. This puts that logic in didUpdate, which is a better place for it.

Changes

• Move logic from willReceiveProps to didUpdate
• Remove some extraneous args from some test functions

Testing

See that the hover, click and reset functionality in the "Reset Rating" example in index.html still work.

Note

I had a couple of tests failing, but they were also failing on master. Not quite sure what was going on there.

[dreyescat]

Thanks @tstirrat15!

I completely agree on the motivation. We must get rid of these deprecated lifecycle functions before the next major version comes out.

Let me try your version tomorrow. I want to check if it works when used as a "controlled" component. I mean, when another component takes over control of the rating value.

BTW:
Excellent pull request summary 👍

[dreyescat]

Is not componentWillUpdate deprecated? Did you mean componentDidUpdate?

[tstirrat15]

🤦‍♂️ yes, you're right. I'll tweak that.

[tstirrat15]

There we go.

[dreyescat]

I had a couple of tests failing, but they were also failing on master. Not quite sure what was going on there.

Don't worry... Test are a little messy right now. They need a good cleaning 😓

[dreyescat]

It's weird but the reset example only works the first time you reset the rating. If you select again after it was reset, then it does not work anymore.

The reset example is a use case of what I like to call "controlled" component behavior. The state value is overriden by the props attribute.

[tstirrat15]

The behavior you're describing looks consistent with the way it's written.

      class ResetRating extends React.Component {
        constructor(props) {
          super(props);
          this.state = {value: 0};

          this.handleClick = this.handleClick.bind(this);
        }

        handleClick(event) {
          this.setState({value: undefined});
        }

        render() {
          return (
            <div>
              <Rating {...this.props} initialRating={this.state.value} />
              <button onClick={this.handleClick}>Reset</button>
            </div>
          );
        }
      }

When you click the button the first time, the initialRating prop changes from 0 to undefined, and the component responds as expected because the prop has changed. When you click the button again, setState is called with value: undefined, so there's a setState and a render, but initialRating hasn't changed because undefined === undefined.

I guess it might be different from the previous behavior because willReceiveProps would run regardless of whether the prop in question had changed, but I'd argue that only updating when the props change is consistent with the conventions of React.

[dreyescat]

This component can be used as an uncontrolled component (the state is managed internally) or as a controlled component (the state is controlled through props by a parent component). willReceiveProps was used to accomplish the controlled mode. My goal is to get rid of the deprecated willReceiveProps method while keeping the controlled mode of functioning.

As an example, I would like to be able to keep the value updated by a parent/wrapper component like this:

class InputRating extends React.Component {
  constructor(props) {
    super(props);
    this.state = {
      value: 1
    };

    this.handleChangeInput = this.handleChangeInput.bind(this);
    this.handleChangeRating = this.handleChangeRating.bind(this);
  }

  handleChangeInput(event) {
    this.setState({ value: event.target.value });
  }

  handleChangeRating(value) {
    this.setState({ value });
  }

  render() {
    return (
      <div>
        <p>
          <input
            type="range"
            min={1}
            max={5}
            value={this.state.value}
            onChange={this.handleChangeInput}
          />
        </p>
        <Rating
          initialRating={this.state.value}
          onChange={this.handleChangeRating}
        />
      </div>
    );
  }
}

Check this example on codesandbox. You can change the rating value either using the actual rating component or the slider. Both components, input and rating components, are being used as controlled components.

Maybe getDerivedStateFromProps is the solution.

BTW:
Sometimes initialRating property name leads to confusion. It is not the first time I have heard complains about the name 😅 (#84 or #108). It is used as the initial value for the uncontrolled version and the value for the controlled version. I mean, when used as an uncontrolled component the state value is initialized to the initialRating, but, when it is used as a controlled component the state value matches to the initialRating.

[tstirrat15]

Have a look now. I agree with other commenters that initialRating is a little misleading, and that may be something worth reexamining in a future iteration of this library - I don't think there's any problem with this library being opinionated about controlled components, since it seems to be considered best practice by the React docs.

If a user wants to create an uncontrolled component using a controlled component as the base, it's not terribly difficult to write your own wrapper.

[dreyescat]

I agree with both the misleading name and the controlled component to be reexamined in a future version.

In fact, I think it is part of how this library has evolved. It started as a closed uncontrolled component where you set the initialRating (previously also named initialValue and initialRate). Then the idea of being controlled from outside using props was added. But the same property was used for both behaviors (hence the misleading name 😓). And now it seems more logical the other way around.

Maybe having value and defaultValue properties would be more evident. The same way input and other elements provide.

[dreyescat]

Ups. this.props -> props
So used to write always together...

[tstirrat15]

🤦‍♂️ good catch.

[tstirrat15]

Yeah, I agree. I think you wouldn't even need a defaultValue - unless you mean like a placeholder?

I think you'd still need the internal state layer of displayValue for the interaction's sake, but the external-facing API would be cleaner.

[tstirrat15]

If you're interested, I could create an issue/PR for moving towards a controlled component API.

[dreyescat]

I have been reading about it. In You Probably Don't Need Derived State it is mentioned as anti-pattern the way it was/is the initialRating property updated.

I would be really open to accept a PR 👍.

[tstirrat15]

Sounds good. I'll put that on my TODO list.