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] FormStateToRedux - Maximum update depth exceeded #335

Closed
borm opened this issue Sep 20, 2018 · 19 comments
Closed

[BUG] FormStateToRedux - Maximum update depth exceeded #335

borm opened this issue Sep 20, 2018 · 19 comments
Labels

Comments

@borm
Copy link

borm commented Sep 20, 2018

All forms with initial values

No nested input names

Original simple example - codesandbox
Original example with connected Container - codesandbox

Nested input names

Bug
Original example with connected Container and nested initialValues & input names - codesandbox

Fix
Original example with connected Container & Form and nested initialValues & input names - codesandbox

@erikras
Copy link
Member

erikras commented Sep 20, 2018

I'm sorry, but I'm not seeing that error when typing in the fields. Can you give me some steps to reproduce?

@erikras
Copy link
Member

erikras commented Sep 20, 2018

Yeah, that's one of the ones above. I clicked on them all. What do I have to do to recreate the error?

@borm
Copy link
Author

borm commented Sep 20, 2018

@borm
Copy link
Author

borm commented Sep 20, 2018

The code is updated

@borm
Copy link
Author

borm commented Sep 20, 2018

fuck, something wrong in my example

@borm
Copy link
Author

borm commented Sep 20, 2018

updated again

const ConnectedReduxContainer = connect((state) => ({
  state: getFormState(state, "example")
}))(Container);

@erikras
Copy link
Member

erikras commented Sep 20, 2018

Ooh!!! This was an interesting one!!

The Form component is only doing a shallow equals check on the initialValues prop to determine if it has changed. If it has changed, then it reinitializes the form, which, in turn, was dispatching the update action via the FormSpy. That's why it works if the initialValues are flat, but not if they are deep.

I think that it probably needs to do a deep equals check on initialValues, but for now you can work around it by not creating a new object of your initialValues every time the connected component rerenders. See lines 20-25:

Edit 🏁 React Final Form - Redux Example

Personally, I would not connect the container of the form to Redux in this way; I would only connect whatever particular deeper component needs that state. Makes sense?

@borm
Copy link
Author

borm commented Sep 20, 2018

Oh, my initialValues depended from BackEnd, it`s cant be static

@erikras
Copy link
Member

erikras commented Sep 20, 2018

That's fine, as long as it's the same object coming from Redux or whatever is loading them. You just can't construct a new object when passing the prop.

That said, I will implement deep equals on the initialValues check.

@erikras erikras added the bug label Sep 20, 2018
@borm
Copy link
Author

borm commented Sep 20, 2018

as example i can define initial values in component constructor, makes sense?

@erikras
Copy link
Member

erikras commented Sep 20, 2018

Yep. As long as the object is only instantiated when they actually change, you should be fine.

@borm
Copy link
Author

borm commented Sep 26, 2018

when you planned fix it?

@erikras
Copy link
Member

erikras commented Sep 28, 2018

Yes, sorry. This week has been busy.

@erikras
Copy link
Member

erikras commented Sep 28, 2018

Okay, so deepEqual() is complicated. I wonder if we could allow an isEqual function to be passed in for initialValues checking....

@arthurdenner
Copy link

arthurdenner commented Oct 1, 2018

@erikras would you consider using react-fast-compare to compare initialValues instead of allowing an isEqual function? Seems like a good alternative to solve the problem and it's pretty small too.

@erikras
Copy link
Member

erikras commented Oct 2, 2018

Published fix in v3.6.6.

@erikras
Copy link
Member

erikras commented Oct 2, 2018

@arthurdenner My reasoning on going with a function prop is that most people won't need it, as most forms are not deeply structured, but to allow flexibility for those that are.

@arthurdenner
Copy link

@erikras Got it. Thanks for explaining it.

@lock
Copy link

lock bot commented Oct 2, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Oct 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

3 participants