on version 2.0.1 ReactRating-component onChange-event seems to return always value of initialRating

[bumpah]

Following occurs,

v2.0.1 => value in onChange-event is always same as initialRating-value https://codesandbox.io/s/react-rating-201-9p75n
v2.0.0 => works as expected https://codesandbox.io/s/react-rating-200-pm4n9

example behavior in CodeSandbox

[dreyescat]

Thanks @bumpah for the sandboxes! That really helps to track down the issue.

Something weird is happening... If you change the react version down to 16.3 it works. From react version 16.4 upwards then the behavior is what you describe.

[dreyescat]

React 16.4 Bugfix for getDerivedStateFromProps might be the reason why it is working in React 16.3 and not from 16.4 and above.

Too look into:

• You Probably Don't Need Derived State

[bumpah]

I believe here is a cultrip on this.props.onChange-call

handleClick(value, e) {
   const newValue = this.translateDisplayValueToValue(value);
   this.props.onClick(newValue);
   // Avoid calling setState if not necessary. Micro optimisation.
   if (this.state.value !== newValue) {
     // If we have a new value trigger onChange callback.
     this.setState({
       value: newValue
     }, () => this.props.onChange(this.state.value)); // this.state.value here does not change
   }
 }

Inspected this a little bit and you'll notice that this.state.value does not get updated as expected and always provides value of a initialRating -value (initial state so to say).

[dreyescat]

Yep. I have just taken a quick look and yes. It is because the getDerivedStateFromProps is called before the setState callback. There, in the getDerivedStateFromProps, the state.value is set to initialRating because, yes, the initialRating is likely to be different than the recently clicked/set value. So, basically, we are always setting initialRating as the current value 😓.

  static getDerivedStateFromProps(props, prevState) {
    const { initialRating } = props;
    return (initialRating !== prevState.value)
      ? { value: initialRating }
      : null;
  }

Roughly this is the sequence of calls:

1. handleClick
2. getDerivedStateFromProps
3. setState callback

In the meantime a solution is found I have deprecated the 2.0.1 version.
I have published the version 2.0.2 that is just a copy of the 2.0.0 to try to cut the bleeding 😉.

[bumpah]

What is the main objective you want to achieve by using getDerivedStateFromProps-function to set initialRating this way?

If you remove that function, component works as I would describe "as you'll expect".

if you want only set initialRating once when component mounts that is already handled by in component constructor function,

    this.state = {
      value: props.initialRating
    };

If you which to achieve something more complex with this that im not currently aware I can collaborate with this issue if given some details.

[dreyescat]

What is the main objective you want to achieve by using getDerivedStateFromProps-function to set initialRating this way?

It is a long story 😉. In short, it is an attempt to replace/get rid of the deprecated componentWillReceiveProps while keeping the same controlled/uncontrolled component behavior.

In #129 we just tried to replace this deprecated life cycle method with the, I thought equivalent, getDerivedStateFromProps method. But, as you can see they are not actually equivalent.

After discussing this replacement in PR #129 with @tstirrat15 and reading You Probably Don't Need Derived State and Always be ready to render posts, we agreed that trying to keep this double behavior (controlled and uncontrolled) brings an unnecessary complexity.

A new issue #134 was created with the goal of refactoring it to meet the currently recommended best practice.

Hope it gives you some context.

And of course, I would be really glad of any kind of collaboration 😃

[dreyescat]

Forgot to actually revert the offending code 😞. The good reverted version is 2.0.3. Apologies.

[zehawki]

After wrangling with this for quite a while, I finally concluded that this must be a bug in react-rating and then a search brought me here. I'm on v2.0.5, so I'm a bit confused whether this is still an issue or sorted in 2.0.3?

[dreyescat]

@zehawki Yep. It should have been fixed on v2.0.3 or greater. The affected versions should only be v2.0.1 and v2.0.2. So v2.0.5 should be fine. Sorry for the confusion it could have caused.

[zehawki]

Well the reason I posted is that I see the issue is there in 2.0.5...

[dreyescat]

Then it should be another reason causing the same issue.

I just forked the offending version from the OP:
v2.0.1 => value in onChange event is always same as initialRating value https://codesandbox.io/s/react-rating-201-9p75n

with the version you say:
v2.0.5 => value in onChange event is as expected https://codesandbox.io/s/react-rating-205-te4ik

and it seems fixed. I mean, the value is changed.

Could you use a fork of this codesandox and try to reproduce your issue?

[zehawki]

Please see this starting point: https://codesandbox.io/s/react-rating-205-forked-n5wps?file=/src/index.js. As soon as there is a real onchange handler, things behave strangely. In this case, it takes 2 clicks to register.

Next: https://codesandbox.io/s/react-rating-205-forked-ffkzz?file=/src/index.js: 1st click resets to initial rating and 2nd click registers the new rating.

[dreyescat]

I think I know what's the problem... It is the evil initialRating name hitting again 😓.

When you are handling the state yourself (like useState does), then initialRating becomes the actual value. I know. The name is misleading and is one of the things we should improve in next versions.

You have to provide the value otherwise the rating will get stuck with the initiaRating. Actually, this, the uncontrolled way, is the way this component should aim to and get rid of the confusing controlled one.

function App() {
  const [value, setValue] = useState(11);

  useEffect(() => {
    console.log(value);
  });

  return (
    <div className="App">
      <ReactRating
        start={6}
        stop={16}
        initialRating={value}
        onChange={setValue}
      />
    </div>
  );
}

Check https://codesandbox.io/s/react-rating-205-forked-qcz09

For more information:

Sometimes initialRating property name leads to confusion. It is not the first time having problems with such a 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.

[dreyescat]

I close this issue because new versions (>= v2.0.3) solve the original bug.

[zehawki]

I feel there's a fair amount of misunderstanding of React going on here. Whether the component is controlled or uncontrolled, it still has to send its value to the outside world else whats the point. The only way it will send its value outside is via an onClick or onChange. And therefore, there is no condition in which one would not use an onChange. And merely because onChange is called, it does NOT mean that: When you are handling the state yourself (like useState does), then initialRating becomes the actual value. I know

Put differently: just because onChange is called, does not mean that the state of this component is being handed from outside, ie controlled. It would be controlled if a props like "value" was send back into this component and actually used. You would see in the code that I've provided, I'm using this component in uncontrolled mode.

Now to correctly get uncontrolled mode working, with an initialValue, all you have to do is this:

1. Get rid of both UNSAFE_componentWillReceiveProps functions. These are not needed.
2. In componentDidUpdate of class Rating, add:

if (this.props.value !== prevProps.value)
      this.setState({ displayValue: this.props.value })

initialRating is just fine, and will continue to work as its intended, ie starting value