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

persistentSubmitErrors has no effect when used together w/ sync validation #3384

Open
unconfident opened this issue Sep 7, 2017 · 9 comments · May be fixed by #3890
Open

persistentSubmitErrors has no effect when used together w/ sync validation #3384

unconfident opened this issue Sep 7, 2017 · 9 comments · May be fixed by #3890

Comments

@unconfident
Copy link

Are you submitting a bug report or a feature request?

A bug

What is the current behavior?

persistentSubmitErrors is being ignored by the reducer handling UPDATE_SYNC_ERRORS action, and any change to any field in a form with at least one sync validator attached erases global form error.

Sandbox Link

What is the expected behavior?

Either there should be a way to update field values programmatically and preserve submit errors, or maybe it's just global error that should be treated differently, but I would expect that if persistentSubmitErrors was set to true and sync validators did not return a different error, the one returned by the server should still be displayed to the user

@unconfident
Copy link
Author

I guess I see why it's made the way it is, why it's trying to re-validate the entire form when one value changes. Some validators may rely on more than one value at the time and change in one field may make whole form valid, so redux-form revalidates everything. I haven't read all of the code, but I assume that is the logic behind current behavior. But the assumption that sync validation should outweight async one is probably wrong.

I'd say server errors should be prioritized, at least with persistentSubmitErrors flag set. Because server almost always knows better what values it wants and which does not comply.

What I tried to do in that componentWillReceiveProps method (updating field values on a failed submit attempt) is a rare, but I believe still a valid use case. In certain situations there might be a need to reset/change some fields when server rejects the form. All I can think of right now is a validation tokens (captchas, 2FA) which expire on time or after submitting, but that's probably not the only case. Even some captcha implementations may want to return a new token next to the error message(s).

In #3380 I suggested introducing a builtin way of resetting certain fields if form is being rejected by the server. It probably would be an easy solution - clear fields, keep the errors, and it would solve my problem, but it's not universal. Maybe instead there should be a way of updating values so that previous validation errors will always outweight the sync validation, regardless of the persistentSubmitErrors setting? Maybe only during onSubmitFail somehow. That would be useful in situations when it's known that user didn't get to see the errors returned by the server, and yet we desperately need to update a field and perhaps to the invalid value. This sounds messy and overly complicated. And.... it may not be possible in the current data flow.

Maybe I'm just trying to do it wrong? Maybe there's a simpler way I'm not seeing?

@imeleschenko
Copy link

And one more thing. It's confused that with the persistentSubmitErrors: true flag submit action happens even if the form is invalid.
I modified unconfident's example to show what I'm talking about:
https://stackblitz.com/edit/react-forms-persistent-submit-errors-arent-persistent-lfkflx?file=components/Form.js.

@unconfident
Copy link
Author

Okay, so this behavior only applies to form-level error and I see no good reason for this to happen. It doesn't do that with field validation.

There are separate keys for syncErrors, asyncErrors and submitErrors, same for warnings but for some reason form-level errors and warnings always being extracted and stored in one separate key: error. Or warning. And because of this it's getting overwritten with sync/async error or lack of it. And to be able to support ignoreSubmitErrors option they already had to introduce additional flag syncError (boolean) which is used in that one condition. And as @imeleschenko pointed out validity state is being completely thrown out of the window when submitting the form with persistentSubmitErrors option set to true, even if errors are still relevant and apply.

Considering how form level errors and warnings are being passed to redux-form and how it's being fetched back from redux-store I don't really see why they couldn't be kept under _error and _warning keys in the appropriate errors/warnings object. This is better than having 3 or 6 pairs of similarly named keys in store, like asyncError and asyncErrors, and that would allow for complex logic when handling these errors differently based on their type.

This bug is a blocker for us and I'm currently trying to fix it all by doing the following:

  • Remove warning, error and syncError keys from the form state structure
  • Update reducers, selectors and everything that references form-level errors and warnings so they would be fetched in such order:
    syncErrors._error || asyncErrors._error || submitErrors._error

And to improve form condition in handleSubmit:

  • Add new field submitErrorsAreRelevant or submitErrorsUpToDate which would be set to false when values change, but persistentSubmitErrors is set to true.
  • Update condition in handleSubmit to take submit errors into account if they still apply

@erikras I'd like to hear your opinion on this. Maybe error/warning were stored like this for a reason?

@anthonator
Copy link

I'm also curious about this. persistentSubmitErrors seems broken to me. Whenever a change action is fired it will clear the errors for the field being changed.

@unconfident
Copy link
Author

I tried to fix it, but accidentally broke some other edge-case behavior covered by test suite and never tried to redesign my solution to account for that. Maybe I'll give it another shot this evening

@anthonator
Copy link

anthonator commented Feb 28, 2018

Looking through the tests I found this.

https://github.com/erikras/redux-form/blob/c1001bb19cfd5c7153cd05dd10d631fdccd972b3/src/__tests__/helpers/reducer.change.js#L286

I was able to get the persistent behavior by passing false and true as arguments 4 and 5 to change. I'm not seeing any documentation around these two arguments so I'm not sure what they do.

@anthonator
Copy link

I'm able to remove persistentSubmitErrors as a reduxForm option when passing in arguments 4 and 5. Persistent behavior is the same. So it seems to me that test is broken as well as the persistentSubmitErrors flag.

@unconfident
Copy link
Author

So this was my take on this unconfident@ac752d3.

I did what I suggested in my previous comment from December 15 - split error key on three different keys for sync, async and submit errors and also added another key signifying whether submit errors are to be respected or if they just kept around because of persistentSubmitErrors and are for display purpose only.

Most of the tests were fixed in another commit on the same branch and for most of them all it took is to rename error keys or add persistentSubmitErrors to expected state.

But there's one test that I didn't fix, because I simply don't know what would be the proper way to address it. Behavior defined by spec just doesn't seem right to me. It was introduced in d84aa7f to fix issue #2244. The code from the issue does not even use persistentSubmitErrors, yet they expect that it would be possible to resubmit form while there's still relevant form wide error present. To allow very "specific" and weird use case.

This comment describes what I would consider correct behavior:

It doesn't try to submit if your form is invalid, and it already knows that it is invalid. If you were to change a field, it would submit again.

but 5 hours later after posting this he changed his mind and decided to make resubmitting of invalid forms possible. ¯\_(ツ)_/¯

I mean, I can see one legitimate use case where this behavior could be desired: displaying network failures or something like code 500 from server through form-wide error, but I also see plenty of better ways to work around it without straight up ignoring presence of that submit error.

Since it was @erikras decision I'd like to ask him to decide once again how this situation should be resolved. Hopefully it wont take over two months to hear from him

@unconfident
Copy link
Author

Hopefully it wont take over two months to hear from him

how naive I was

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants