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

initialValues overwrites existing form value #370

Closed
qnm opened this issue Dec 2, 2015 · 34 comments
Closed

initialValues overwrites existing form value #370

qnm opened this issue Dec 2, 2015 · 34 comments

Comments

@qnm
Copy link
Contributor

qnm commented Dec 2, 2015

My form uses destroyOnUnmount: false to persist values across route changes but, when a user leaves the page and eventually returns to this form, initialValues will overwrite the existing user data.

Is this by design or a bug? initialValues suggests to me the values will only be used initially and ignored after user input.

If the latter, I'm happy to code up a test case and solution, otherwise I'll guess I'll have to find a work-around.

@erikras
Copy link
Member

erikras commented Dec 2, 2015

This is an interesting edge case. There are two things going on.

  1. The INITIALIZE action sets both the value and initial keys in the store. I think this is the correct behavior.
  2. All the initialValues prop does is dispatch an INITIALIZE action on componentWillMount, which is also think is correct for almost all cases.

However, perhaps initialValues should add an additional noOverwrite boolean to the INITIALIZE action that prevents it from overwriting the value if it finds one already there. What I can't decide is whether or not that should be the default behavior, or if you should have to pass a noOverwriteOnInitialize={true} prop.

Thoughts?

@qnm
Copy link
Contributor Author

qnm commented Dec 2, 2015

However, perhaps initialValues should add an additional noOverwrite boolean to the INITIALIZE action that prevents it from overwriting the value if it finds one already there.

Yes - that makes sense. We might need to re-consider the name through, noOverwrite: false needs a little thought to understand whether or not an overwrite may take place!

What I can't decide is whether or not that should be the default behavior, or if you should have to pass a noOverwriteOnInitialize={true} prop.

As you've stated, this is an edge case, and only really affects forms using destroyOnUnmount: false.

If folks are using destroyOnUnmount: false to support multi-step forms, I'd assume that most multi-step forms allow users to move backwards in the process.

From this, my assumption is that most people using destroyOnUnmount: false in a multi-step form would want noOverwrite behaviour by default.

Additionally, the name initialValuessuggests it's only used initially!

My vote is for noOverwite to be the default behaviour.

@webmasterkai
Copy link

I'm having the same issue and found the initialValues name counterintuitive. I too thought it was only used initially. I was wanting to save the values if the user happens to navigate away from the page and later comes back to edit/save. Why would you ever want initialValues to overwrite the previously set value keys? I could maybe understand overwriting the initial keys, but not value.

If a form has already had the INITIALIZE action run on it, why should it ever run that action again on a previously initialized form? Shouldn't an init function only run once? I would argue componentWillMount is a fine place to call the INITIALIZE action creator but the action creator should check to see if the form has been initialized before running INITIALIZE.

The noOverwrite thing is superfluous.

@erikras
Copy link
Member

erikras commented Dec 4, 2015

You make a good point, @webmasterkai.

@morgante
Copy link

morgante commented Dec 8, 2015

👍 I was quite surprised to find initialValues overriding my form.

@nicgordon
Copy link

+1 for this. Especially @webmasterkai's comments. Spot on.

@ptim
Copy link

ptim commented Dec 14, 2015

subscribing...

@bangn bangn mentioned this issue Dec 15, 2015
@erikras
Copy link
Member

erikras commented Dec 15, 2015

This should be fixed in v4.0.0. Come and get it!

@erikras erikras closed this as completed Dec 15, 2015
@webmasterkai
Copy link

Thank you @erikras 🎉 😄

@qnm
Copy link
Contributor Author

qnm commented Dec 16, 2015

Nice one, thanks @erikras !

@nicgordon
Copy link

🙌 gg

@duro
Copy link

duro commented Dec 17, 2015

@erikras: As it turns out, I was actually using initialvalues in a way where if it changed, I actually did want it to overwrite my form.

For example, I have an edit page that is using a form to edit a DB object. I initially fetch the object in a different store, and on form init use initialValues to pass in the data that was fetched. then the user edits the form, I take the values from the form, use it to save a new object, and then update the original store I was referrign to, which re-renders the component that implements the form, passing the new response returned from the server to the Form component.

I know this is a weird use case, as the new state should be the same as what the form already has, but it just makes me feel better that the form is getting updated with the values that just came off the server from the save.

Is there something that I can use when implementing my form component that will get me back to the previous functionality prior to 4.0.0 that still leaves everyone else happy with the new more sensible implementation of initialValues?

@qnm
Copy link
Contributor Author

qnm commented Dec 17, 2015

@erikras I knew someone would use the edge-edge case!

@morgante
Copy link

@duro: Once the server responds, couldn't you just call initialize on the form?

@erikras
Copy link
Member

erikras commented Dec 17, 2015

"No one would ever expect _____ behavior!" is always a false statement. 😄

@duro, what if the library made you call dispatch(initialize(form:String, data:Object, fields:Array<String>)) manually, instead of the initialValues prop, and that initialize() action creator gave you an overwriteValues boolean option as well?

@erikras erikras reopened this Dec 17, 2015
@box-turtle
Copy link

I have a grid and a form side-by-side in a component. When the user clicks a row, I was grabbing the rowdata and calling setState to push it into the initialData prop of the form. That loads the data into the form as expected. But now with this change in behavior, if the user clicks a different row, what is the appropriate way to load the new data into the form? Passing it into intialData doesn't work now since the form has already been initialized.

@pavelvolek
Copy link

@erikras definitely +1 for the overwriteValues option on the initialize() action creator!

@haradakunihiko
Copy link
Contributor

IMHO, initialValues should change only initialData, and initialize action creator and initializeForm props should change not only initialData but also value itself. Isn't it literally 'initialize' mean?

However in my app, I would be very happy if the value is overridden when I pass different initialValues as props because of the same reason as @duro.

So, as you mentioned,

However, perhaps initialValues should add an additional noOverwrite boolean to the INITIALIZE action that prevents it from overwriting the value if it finds one already there. What I can't decide is whether or not that should be the default behavior, or if you should have to pass a noOverwriteOnInitialize={true} prop.

How about adding overwriteOnInitialize={true} prop with initialValues?

@erikras
Copy link
Member

erikras commented Dec 22, 2015

I think the problem here is that I missed the all-important last big of @webmasterkai's insightful comment.

...but the action creator should check to see if the form has been initialized before running INITIALIZE.

Okay, how about this:

  • We keep track of whether or not a form has been initialized ever, and we only initialize on mount if it's the first time. This will fix @qnm's original problem.
  • We change it back to overwrite value as well as initial when INITIALIZE is dispatched, which I think is the most expected behavior.

The initialValues prop provides two functions:

  1. To set the value of all the fields on mount.
  2. To set the initial for each field to calculate dirty/pristine state.

I would think that the rarest case would be where you need to change initial but not value, so as to change the dirty/pristine state of existing fields.

That sound good?

@duro
Copy link

duro commented Dec 22, 2015

@erikras will this require a manual fire of INITIALIZE when the initialValues prop changes in the component? Or will redux-form handle that?

@morgante
Copy link

@erikras that doesn't seem quite right?

I have default values (set via initalValues) which I want to be used as default values. They should never overwrite the existing values, even if the form has been unmounted and remounted (assuming I have destroyOnUnmount set to false).

@webmasterkai
Copy link

If someone wants to reinitialize can they just run destroy first?

@box-turtle
Copy link

That's what I changed my code to do, destroy and then intitialize if I want
to load a new record in the same form. Seems to work fine and I guess makes
semantic sense.

On Tue, Dec 22, 2015 at 1:56 PM, Kai Curry notifications@github.com wrote:

If someone wants to reinitialize can they just run destroy first?


Reply to this email directly or view it on GitHub
#370 (comment).

@yleclanche
Copy link

The erikras solution would works for me. I have several forms in different pages/tabs (with react-router)
When the user navigate to another tab, the form is submitted and the errors (if any) are set in the form state.
But when the user (damn user) comes back to the form with an error, the errors are not displayed because the initializeState keeps the value but reinitialize the other props (errors, touched...)

@erikras erikras closed this as completed in 09f2186 Jan 2, 2016
@erikras
Copy link
Member

erikras commented Jan 2, 2016

Released this fix as v4.1.0. Come and get it!

@lsjroberts
Copy link

@erikras Thanks, that may help with #439. I'll investigate it tomorrow at the office to see and will post an update on that ticket.

@erikras
Copy link
Member

erikras commented Jan 25, 2016

Hi guys. Just wanted to call to your attention the side effects this decision has caused (linked to this issue above), and possible reopen the debate as to the best behavior.

@morgante
Copy link

@erikras Which issue are you referring to? #588 seems like intended behavior... if you want to reset a form, you should call reset.

The fact is that initialValues does not make any sense at all without this ticket. If it will arbitrarily reset form fields and override user input, it's impossible to use in practice.

@simonedavico
Copy link

@erikras I'm struggling to understand what is the current behaviour of the library. With 6.5.0 I am observing that even if destroyOnUnmount is set to false, the INITIALIZE action gets called when the form is unmounted. I am trying to circumvent this by manually calling initialize in my submission callback, but unfortunately the INITIALIZE action dispatched on unmount ignores the new initial values and always seems to use the ones provided to the reduxForm configuration.

Am I missing something here?

@gustavohenke
Copy link
Collaborator

the INITIALIZE action gets called when the form is unmounted

Hmm, no, this is not happening. Check this example with Redux DevTools open.

unfortunately the INITIALIZE action [...] ignores the new initial values and always seems to use the ones provided to the reduxForm configuration.

That could be because of enableReinitialize option. Try setting it to true.

@simonedavico
Copy link

@gustavohenke you are right, the INITIALIZE action I was seeing was the one for the form on the new page (every page in my app has a similar form).

Nevertheless, when I go back to a form which I previously reinitialized with new values by using the initialize action creator, it is still initialized with initialValues on mount. I have configured enableReinitialize: true, keepDirtyOnReinitialize: true and destroyOnUnmount: false.

I worked around the issue by abstracting away some logic for all my forms:

class FiltersScaffolding extends React.Component {

  constructor(props) {
    super(props);
  }

  componentWillMount() {
    const { initialize, formValues, filtersDefaults } = this.props;
    formValues === undefined && initialize(filtersDefaults);
  }

  render() {

    const {
      children,
      isFetching,
      dirty,
      handleSubmit,
      submitForm,
      initialize
    } = this.props;

    return (
      <form className="stqts-filters"
            onSubmit={handleSubmit(values => submitForm(values).then(() => initialize(values)) )}
      >
        <div className="filters">
          { children }
        </div>
        <div className="filters-aside">
          <SubmitFiltersButton filtersDidChange={dirty} />
          <FiltersLoader isLoading={isFetching} />
        </div>
      </form>
    );

  }

}

export default connect(
  (state, ownProps) => {

    const formValues = getFormValues(ownProps.form, state => state.filters)(state);

    return {
      ...ownProps,
      formValues
    };

  }
)(FiltersScaffolding)

in componentWillMount I manually initialize the form if formValues is undefined.

@ccnixon
Copy link

ccnixon commented Jul 23, 2017

Is anyone else still having issues here? I have tried everything but my form continues to be re-initialized if initialValues changes and all previous user input gets overwritten.

My form has an input field into which the user puts an ID for a Salesforce object. Once one is entered and submitted, the program makes an api call and returns some data. I'm hoping to use this data to auto-populate other parts of the form. This works but it destroys all previous user input.

If I pre-set the initialValues with dummy data, everything works. The issue arises when initialValues is first undefined (before the user has submitted the lookup ID) and then becomes defined after the return of the API call.

Any help or advice would be hugely appreciated. Thanks!

@eliseumds
Copy link
Contributor

eliseumds commented Aug 28, 2017

@ccnixon I have the exact same issue and it's quite problematic in terms of user experience because my form is automatically submitted after any change (with a debounce), so it feels very annoying to have a text field changing while typing. Setting one of the fields to an empty string as default value in the initialValues object solved the issue:

initialValues={{
  name: '',
  ...query
}}

By the way, the issue also persists if initialValues is {} or null (besides undefined).

It apparently has to do with this block of code: https://github.com/erikras/redux-form/blob/42a9c7e71d859b7f06d32fed97288aa5ebbe6a43/src/createReducer.js#L324-L328

@erikras is there any way to override this behaviour?

@lock
Copy link

lock bot commented Jan 13, 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 Jan 13, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests