fix(internalSetState): dont change references inside setState updater #423
Conversation
|
Thanks for the PR!
This just changes the references to objects. As far as I know, |
|
Hmm, when I was running things locally in StrictMode, these two lines seemed suspect to me: stateToSet = isStateToSetFunction ? stateToSet(state) : stateToSet
stateToSet = this.props.stateReducer(state, stateToSet) The stateToSet reference gets changed to an object with the new state after the first line. Since stateToSet is in the closure and outside this scope, it remains changed on subsequent calls to the updaterFunction. The first time this is invoked, things work fine. But the second time, before my change, stateToSet was an object and not a function. Does that make sense? |
|
@kentcdodds - I added a failing test case. This test passes with my changes and does not without. The behavior I was seeing in our Application occurred during a call of reset() so I mimicked that in the test. Hope this helps, let me know if theres anything else I can provide. |
| @@ -116,6 +116,22 @@ test('can use children instead of render prop', () => { | |||
| expect(childrenSpy).toHaveBeenCalledTimes(1) | |||
| }) | |||
|
|
|||
| test('should not throw error during strict mode during reset', () => { | |||
There was a problem hiding this comment.
Thanks for this! Do you mind showing me this test failing without your changes? I'm really sorry, I'm just struggling to see how your changes really impact anything here :-/
There was a problem hiding this comment.
How would you like me to do that? Would you like me to revert my change in downshift and push? I can do that first thing tomorrow when I get to work.
There was a problem hiding this comment.
A screenshot of the failing test would suffice.
Or if you could point me to the part of your code that's making the real impact or explain how this is making a difference. I'm sure I'm missing something, but it looks like this is just renaming variables...
There was a problem hiding this comment.
@kentcdodds - Here is a screenshot of the failing test.
Let me try to explain a little more of the issue.
The internalSetState function is not invoked twice by react when in Strict mode. The part that is invoked twice is the updater function that is passed to this.setState. The first time the updater function is invoked, stateToSet is a function (which was passed into internalSetState). Inside the updater function, the stateToSet variable gets reassigned to the result of that function call which is an object (the new state). React then invokes the updater function again (as part of StrictMode). This time, stateToSet is an object since that was changed during the previous invocation.
My change ensures that stateToSet variable never gets reassigned so that it remains the same on subsequent calls of the updater function.
There was a problem hiding this comment.
Here is a CodeSandbox of the issue if this helps:
|
Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the |
|
🎉 This PR is included in version 1.31.8 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
|
@kentcdodds - No problem! Happy to contribute. Glad I was able to demonstrate what was going on for you. Some times things are tough to articulate :) I'll gladly look over those guidelines and contribute where/when I can. This component has been a huge win for us and we would love to give back where we can. Thanks! |
…downshift-js#423) * fix(internalSetState): dont change references inside setState updater function * fix(internalSetState): remove change to arg passed to updater function * fix(internalSetState): add failing test case
What:
This fixes an issue when using the new React 16.3 React.StrictMode component which invokes setState updater functions twice. This resolves issue #403
Why:
This change is necessary in order to resolve a runtime error when using React.StrictMode. (Note: StrictMode does not affect production code, only development).
How:
This change changes the behavior in the setState updater function by creating new references to state and the stateToSet that is in the closure.
Checklist:
I wasn't sure if there were any additional tests that needed to be added. Let me know if there are anything and I can try and add those.