react-compound-slider icon indicating copy to clipboard operation
react-compound-slider copied to clipboard

Bug in React Experimental Build

Open Dr-Nikson opened this issue 5 years ago • 2 comments

Problem or feature description:

In some cases onChange callback is fired during rendering (especially with new react' concurrent mode)

Steps to reproduce (for problems):

Versions (for problems):

React-Compound-Slider: 2.4.0

React: experimental

Performing side-effects inside getDerivedStateFromProps is antipattern (due to official react docs)

So, this lines of code blowing react is some cases. Because these callbacks occasionally can be fired during parent' render stage.

Dr-Nikson avatar Dec 04 '19 17:12 Dr-Nikson

Hey, thanks for pointing this out. I'm planning to port this to hooks so this should not be an issue.

sghall avatar Dec 06 '19 01:12 sghall

It seems to me like a similar issue also is present on the stable react v16.12.0. A Slider would fire the onChange handler when mounted, which was an issues for me as I needed to differentiate between the pristine state and one where actual change happens., while still handling updated min/max values coming from a prop correctly. The call would fire with both values reflecting the minimum value of the two values it recieved.

I forked the Slider.js component, adjusted imports and commented out both the lines @Dr-Nikson mentioned aswell as the other onChange/onUpdate Handler in getDerivedStateFromProps, so: https://github.com/sghall/react-compound-slider/blob/master/src/Slider/Slider.js#L119-L122 and https://github.com/sghall/react-compound-slider/blob/master/src/Slider/Slider.js#L137-L140

Now that error is gone, while the component still seems to be functional and deals with updated values correctly.

Probably won't matter when the logic is reimplemented in hooks, but in the meantime maybe someone has to deal with the same issue and reads this.

janus-reith avatar Dec 06 '19 20:12 janus-reith