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

[6.2.0] initialize(currentFormData) clears warnings and errors until the next onChange #2142

Closed
bmv437 opened this issue Nov 17, 2016 · 10 comments · Fixed by #2151
Closed

[6.2.0] initialize(currentFormData) clears warnings and errors until the next onChange #2142

bmv437 opened this issue Nov 17, 2016 · 10 comments · Fixed by #2151

Comments

@bmv437
Copy link
Contributor

bmv437 commented Nov 17, 2016

Ok, so I've tracked this bug all the way down to the cause, I just don't have a solution. I'd be happy to submit a PR if we can figure out a solution.

A little background:
in src/reduxForm.js, warnIfNeeded() will always fire updateSyncWarningsIfNeeded() on initialRender (nextProps === undefined), but on subsequent renders, it will only fire the updates if !deepEqual(values, nextProps.values)

see the following copy of warnIfNeeded

warnIfNeeded(nextProps) {
  const { warn, values } = this.props
  if (warn) {
    if (nextProps) {
      // not initial render
      if (!deepEqual(values, nextProps.values)) {
        const { _warning, ...nextSyncWarnings } = warn(nextProps.values, nextProps)
        this.updateSyncWarningsIfNeeded(nextSyncWarnings, _warning)
      }
    } else {
      // initial render
      const { _warning, ...nextSyncWarnings } = warn(values, this.props)
      this.updateSyncWarningsIfNeeded(nextSyncWarnings, _warning)
    }
  }
}

this makes sense for most cases, but what if you initialize the form using the current values of the form (in order to get a pristine state)?

const warn = (values) => {
  let warnings = {};
  if (!values.message || !values.subject) {
    warnings._warning = "missing required fields";
  }
  return warnings;
}

class MyFormComponent extends Component {
  handleSave = (data) => {
    this.props.saveData(data);
    this.props.initialize(data);
  }

  handleSend = (data) => {
    this.props.sendData(data);
  }

  render() {
    const {pristine, warning, handleSubmit} = this.props;
    return (
      <div>
        <Field name='subject' component='text' />
        <Field name='message' component='text' />
        <button type='button' 
                disabled={pristine} 
                onClick={handleSubmit(this.handleSave)}>
          Save Draft
        </button>
        <button type='button' 
                disabled={!!warning} 
                onClick={handleSubmit(this.handleSend})>
          Send
        </button>
      </div>
    );
  }
}

const MyForm = reduxForm({
  form: 'myForm',
  warn
})(MyFormComponent);

Since the action handler for the initialize action starts from an empty state, warnings get cleared from the state. and since it's not initialRender, and the values of the form haven't changed, it won't re-calculate warnings.

We need a way to the form to recalculate warnings even if the form has been initialized to it's current values in order to achieve a pristine state.

Technically there is a similar issue with sync errors and the validateIfNeeded() function, but you can't fire a submit on a form with errors anyway (hence why I'm using warnings).

I'd love to get some discussion going around a solution, and like I said, I'd be happy to submit a PR with the final fix.

@erikras
Copy link
Member

erikras commented Nov 17, 2016

Dropping the if (!deepEqual(values, nextProps.values)) would do it, right? I wonder how much efficiency that's really providing... It would only be important if the warn() function was cpu-heavy.

@bmv437
Copy link
Contributor Author

bmv437 commented Nov 18, 2016

after messing around with this for the later half of the day, removing that !deepEqual check doesn't solve it, that's just the first barrier.

The second barrier exists inside updateSyncWarningsIfNeeded where it checks to see if the warning is different from the previous warning. It won't pass that condition since it's comparing this.props.warning to the recalculated nextWarning, which are the same. So it will end up failing that and not firing updateSyncWarnings().

The only solution I've come up with is to modify the way the INITIALIZE action handler works.
currently it starts with a blank slate. I propose we follow that line with the following:

...
    var result = empty; // clean all field state
    // keep old warnings, they will get recalculated if the form values are different from current values
    const warning = getIn(state, 'warning')
    if (warning) {
      result = setIn(result, 'warning', warning)
    }
    const syncWarnings = getIn(state, 'syncWarnings')
    if (syncWarnings) {
      result = setIn(result, 'syncWarnings', syncWarnings)
    }
...

This will persist the old warnings through the INITIALIZE action, which solves my test case where you are reinitializing the form with its current values. It will also not break the current functionality when you reinitialize with values that are different than the current form values, since that passes the first barrier, and will go and re-calculate warnings.

I've tested it locally with both cases and it seems to work fine, and it passes all current tests. Thoughts? I can whip up a PR if you approve.

@erikras
Copy link
Member

erikras commented Nov 21, 2016

makeitso

@erikras
Copy link
Member

erikras commented Nov 30, 2016

Fix published in v6.2.1.

@dfalt
Copy link
Contributor

dfalt commented Dec 6, 2016

Just added PR #2228 which solves the other part of this issue (errors not being persisted across initializations)

@bmv437
Copy link
Contributor Author

bmv437 commented Dec 6, 2016

@dfalt The reason I didn't add that in as well is because it to initialize() a form with the same values if it has errors, since the form can't be submitted if it has errors (handleSubmit() won't fire if the form has errors).

The only case where this would be possible is if you happen to fire initialize() outside of handleSubmit() that just happens to have the same values.

@dfalt
Copy link
Contributor

dfalt commented Dec 7, 2016

@bmv437 You're right, after some more digging this is not a re-initialization issue. It actually appears to be an issue with the form's first initialization. I should probably open up a separate issue, but I'll clarify what I'm seeing here anyways.

This issue is manifesting for a form which loads its initialValues asynchronously and sets them after the redux-form component has been mounted.

  1. During redux-form's componentWillMount call, validateIfNeeded is called
  2. Since this is the initial render, the redux-form determines that we should call the form's validate method, which returns errors and ultimately sets these errors through the UPDATE_SYNC_ERRORS action
  3. A bit later, initialValues are passed in as properties causing componentWillReceiveProps to be called
  4. Since the form has yet to be initialized, the INITIALIZE action is fired, setting the form's state to include initial and values (and thereby removing the syncErrors that it had found originally, which is okay until...)
  5. redux-form updates the component's values prop which triggers componentWillReceiveProps and thereby validateIfNeeded to be called
  6. since the component's values have changed, we re-validate and find that there are errors which we pass into updateSyncErrorsIfNeeded
  7. here we determine that the sync errors we found are the same that are stored in our component's syncErrors props, so we do not call the updateSyncErrorsaction creator

The problem lies in step 7 here. Our component is still aware of the original sync errors that we found in step 2, while our redux form state thinks there are no errors.

Perhaps there is a better place to put the fix, but by making sure errors can pass through initialization (exactly how you solved with warnings) we solve the problem as well.

@mvirbicianskas
Copy link

mvirbicianskas commented Dec 9, 2016

any progress on this? i've the same prob as dfalt explained, sync errors if they're same they don't pass through initialization ;/

Edit: i've updated to v6.3, but the problem in my case, when the form is reinitialized the let formErrors = getFormSyncErrors('MenuForm')(state); is not updated, but if i understand correctly reinitialization shouldn't do that anyways?

@dfalt
Copy link
Contributor

dfalt commented Dec 9, 2016

@mvirbicianskas this particular fix has been released in v6.3.2, however, I'm not sure I'm following what's going on in your example. I'd suggest upgrading to v6.3.2 to see if that does resolve your issue and if not perhaps provide some more information about your setup.

How is it that you are reinitializing your form? Are you reinitializing with the same form values as it was initialized with or new form values?

@lock
Copy link

lock bot commented Jun 2, 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 2, 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 a pull request may close this issue.

4 participants