Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug in React Experimental Build #106

Open
Dr-Nikson opened this issue Dec 4, 2019 · 2 comments
Open

Bug in React Experimental Build #106

Dr-Nikson opened this issue Dec 4, 2019 · 2 comments

Comments

@Dr-Nikson
Copy link

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.

@sghall sghall changed the title Bug with calling onChange handler Bug in React Experimental Build Dec 6, 2019
@sghall
Copy link
Owner

sghall commented Dec 6, 2019

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

@janus-reith
Copy link

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants