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

move initialization to componentDidMount #3615

Merged
merged 1 commit into from
Nov 19, 2017

Conversation

brucewpaul
Copy link
Contributor

React 16 ensures a certain order of lifcecycles when replacing a
component which causes redux-form to destroy the form, breaking
it. This change, when used with enableReinitialize, causes the
form to reinitialize correctly, preserving the form.

this pr address #3435

this is probably going to fail a lot of tests, but want to start the
conversation as to what is needed for redux-form to work with
React 16

@brucewpaul
Copy link
Contributor Author

cc @erikras @gustavohenke

@brucewpaul
Copy link
Contributor Author

actually, just copying the same functionality form componentWIllMount to componentDidMount passes all the test and is working in our app. Although this solution of course comes with the added perf costs which isn't ideal at all...

@gustavohenke
Copy link
Collaborator

Well, actually, quite a few tests are breaking!
https://travis-ci.org/erikras/redux-form/jobs/302635057#L6766

@brucewpaul
Copy link
Contributor Author

:) @gustavohenke this pr doesn't include the most recent note I wrote. Locally, all the tests are passing. I will update this pr to be that version since all the tests pass

React 16 ensures a certain order of lifcecycles when replacing a
component which causes redux-form to destroy the form, breaking
it. This change, when used with `enableReinitialize`, causes the
form to reinitialize correctly, preserving the form.
@brucewpaul
Copy link
Contributor Author

@gustavohenke updated and the tests just passed locally

@codecov-io
Copy link

Codecov Report

Merging #3615 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3615   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          70      70           
  Lines        1558    1563    +5     
======================================
+ Hits         1558    1563    +5
Impacted Files Coverage Δ
src/createReduxForm.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df718b8...74e31ed. Read the comment docs.

1 similar comment
@codecov-io
Copy link

codecov-io commented Nov 15, 2017

Codecov Report

Merging #3615 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #3615   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files          70      70           
  Lines        1558    1563    +5     
======================================
+ Hits         1558    1563    +5
Impacted Files Coverage Δ
src/createReduxForm.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update df718b8...74e31ed. Read the comment docs.

@stevenmusumeche
Copy link

is it likely that this PR will be merged? I have a workaround using setTimeout that I'd rather not implement if I don't have to.

@brucewpaul
Copy link
Contributor Author

brucewpaul commented Nov 16, 2017

@stevenmusumeche could you give an example of the workaround?

@stevenmusumeche
Copy link

In my case, I am navigating AWAY from the form, and when I go back to it, it already has "submitSucceeded" set to true from the previous submission. So, I was able to do a setTimeout and call destroy on the form after 1 second. Dirty hack, but it works for my use case.

@brucewpaul
Copy link
Contributor Author

@stevenmusumeche does this solution work for you?

@stevenmusumeche
Copy link

This PR? I have not run my code with it, sorry.

@erikras
Copy link
Member

erikras commented Nov 19, 2017

Sorry guys. I was sick last week.

Meh, I don't care about "perf costs" when hot reloading. Looks good.

@erikras erikras merged commit 36773db into redux-form:master Nov 19, 2017
@brucewpaul
Copy link
Contributor Author

@erikras I was experiencing this in production, not just hot reloading, which is why I brought up the perf cost of this change.

@stevenmusumeche
Copy link

Yes, same here, it's not just a hot reloading issue.

@brucewpaul
Copy link
Contributor Author

@erikras would you be able to do another release that includes this?

@erikras
Copy link
Member

erikras commented Nov 27, 2017

Published in v7.2.0.

@lock
Copy link

lock bot commented Jun 1, 2018

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 Jun 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants