-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Remove setState util call in Form.onChange #1297
Conversation
I see related issues/comments regarding safeRenderCompletion, but setting that to true didn't resolve this bug. I see someone said "On slow CPUs safeRenderCompletion needs to be set to make sure RJSF waits for setState before calling onChange" (#544) but I don't see their argument of "waiting" for setState -- like what exactly are you waiting for? The way I see it, if an underlying component's UI/internal state changes, it needs to propagate that event back to its parent/orchestrator so if the parent wants to act on the change (such as change props and therefore cause the child to recompute a new derived state) the parent can use the new local state/value to call its own setState which will feed back into the props of the child component. However, in the case of the RJSF Form, formData prop is optional so you can let the component maintain its own state, and thus, its own local state needs to be updated when its children/widgets update their local state so it can feed that value back down to them for consistency. I also saw #1197 which talks about doing away with But I'm pretty new to React so idk, I don't really understand the design decisions made here, but this is the 'fix' I found. Also saw some mention of a V2 coming up so I hope |
This PR might not have a huge problem with backwards incompatibility though, given that #1068 was merged a few months ago. |
Made a comment at the affected line in the PR, possible race condition |
@epicfaace I actually had race condition issue with the way it was (props.onChange and then setState) on a Form wrapper I had made, so I'm assuming that the underlying Form itself probably needs to setState and then call props.onChange. I think this is actually the current implementation's control flow anyway since the arrow function is the callback for after React.setState is actually called: Old code/current implementation:
The weird setState helper/util:
|
Background: In my (multi-layer) wrapper of the rjsf Form component, I had:
at each layer except one componentDidMount lifecycle in one of the layers. This caused a bug with 1 out of about 20-25 use cases (all pretty similar so by no means a thorough test suite), and just swapping the order to setState -> props.onChange fixed that one Form and caused no regression in the others so assuming that it's always the right thing to do.
so end-user can optionally not provide listeners/continuous state management and rely on the Form to act as a self-managed form. |
src/components/Form.js
Outdated
} | ||
}); | ||
|
||
this.setState(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering, why not do something like this:
this.setState(state); | |
this.setState(state, () => this.props.onChange && this.props.onChange(state) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh so that's what you and @sbusch meant by race condition -- using the callback to call props.onChange after a re-render completes/update to state has been applied. As noted by the docs (https://reactjs.org/docs/react-component.html#setstate) the pitfall is when you're this.state
since it won't reflect the change you're about to/attempting to do. However, as this an instanced variable not tied to this.state
, I don't think such a "pitfall" is possible here. Instead, what I think would happen is that the setState
may defer the update to the component until the 1st thread's execution is complete and, thus, props.onChange
may be called before the Form
updates its own state. Consequentially the parent/user could end up acting on a change event and change the props of the Form
initiating another update to the form (based on the future state) -- so the race condition could be that state updates with a newProps
with this.state = oldState
, and then updates again with just the new state (potentially discarding any derived state updates from a change in props and the new current state (which was pretty much the bug I had mentioned when I used props.onChange
before this.setState()
).
I think not using the callback and allowing React to batch the updates (to the just the state I believe -- it can't possible have a reference to oldProps and newState, right? https://reactjs.org/docs/state-and-lifecycle.html#state-updates-may-be-asynchronous) is fine (though I'm no React expert, just giving my 2 cents) if there aren't such hooks that compare newProps
vs oldProps
and the current this.state
(though that's what one of my Form wrappers has to do and caused an issue where it reset the values).
So sure, I guess using callback to slow it down and force everything to be synchronous may be best/least messy course of action, even if it potentially increases the number of times render() is called.
I think we definitely want to merge this and remove the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epicfaace IMO the transition to proper use of setState()
is worth risking some possible problems. Most of them - if any - should be not too hard to fix.
At some time, the original problem - that caused the custom setState
wrapper - will arise again: performance regressions. But they need to be optimised with proper React methods anyway (immutable data, memoization, pure components, ...)
src/components/Form.js
Outdated
}); | ||
|
||
this.setState(state); | ||
this.props.onChange && this.props.onChange(state); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could cause race conditions when React becomes more and more async. Basically, the onChange
could possibly be called before the new state has been applied internally to the component. onChange
would correctly get the new state (as it is given as a param), but the components internal state may differ.
The correct way is to use the callback React's setState
offers (similar to our own, hackish setState
), see here:
this.setState(state, () => this.props.onChange && this.props.onChange(this.state));
@mirajp Can you make this PR off of the v2 branch so we can remove all setState util calls in v2? |
@epicfaace Made #1454. |
Closing this in favor of #1454. |
Reasons for making this change
Remove the util setState call from Form.onChange because it sets the state too early or something? Hard to describe, I came upon this fix (for #1281) after trying a bunch of things like using a leaner
componentDidUpdate
and removingshouldComponentUpdate
(so it uses the default React Component function which always returns true) instead of the existingcomponentWillReceiveProps
andshouldComponentUpdate
. Note that removingcomponentWillReceiveProps
breaks a lot of existing tests because for some reason they rely oncomponentWillReceiveProps
(not sure why tests call that function directly, but I'm just a little more used to Jest than the raw 'react-addons-test-utils' library), so that's not an option anyway.I'm not sure how to add tests for the bug I'm trying to fix, so feel free to edit this PR.
If this is related to existing tickets, include links to them as well.
#1281
Checklist
npm run cs-format
on my branch to conform my code to prettier coding style