Skip to content

Conversation

@rkuykendall
Copy link
Member

( what is the feature, or what is it this is PR fixing? )

  • Title is human readable, as it will be included directly in CHANGELOG.md when we do a release
  • If this is a new user-facing feature, documentation has been added to API.md
  • Any dist/ changes have not been committed.

Tests run directly on the source code and all dist changes are computed and committed during Formsy release.

Copy link

@adsee42 adsee42 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great👍
Didn't know about lodash's get and set.😋

btw, (ignore this if I was wrong) this function looks and works very similarly to updateInputsWithError, and how they are implemented seems different. wondering why is the difference

@rkuykendall
Copy link
Member Author

@adsee42 You're right. I thought a lot about this and I'm on the fence. Should you be able to pass object-style error messages like { foo: bar: 'Invalid phone number' } map to the input foo.bar, or should, when setting error messages, you be required to supply the real name of the field?

The inputs with value one makes more sense, since we output an object at the end.

Of course none of this would matter except that updateInputsWithError throws an error if you supply a string that's not the name of the input, and that feature doesn't make sense if we support deep-error-objects, so we would want to remove it, which would be a "breaking" change, and therefore requires more thought.

@adsee42
Copy link

adsee42 commented Dec 24, 2020

@rkuykendall
I understand.
Though, why throwing errors if the names do not match? I suppose people using updateInputsWithError with their customized error object (since different servers or api return different errors), and they would know if they didn't see the error messages supposed to show up?

@rkuykendall
Copy link
Member Author

Yeah it feels like an odd feature. I guess the idea is that if you're passing errors to the form and they're not in the form, that's bad and we should let you know.

@rkuykendall
Copy link
Member Author

I'm going to merge the change as-is and leave the questions about input errors for another conversation.

@rkuykendall
Copy link
Member Author

@ThibautMarechal or @felixmosh Can I get a code review if you're free?

Copy link
Contributor

@felixmosh felixmosh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK to me :]

@rkuykendall rkuykendall merged commit be68616 into master Jan 21, 2021
@rkuykendall rkuykendall deleted the deep-update branch January 21, 2021 16:59
@rkuykendall rkuykendall restored the deep-update branch January 22, 2021 15:26
@rkuykendall rkuykendall deleted the deep-update branch January 22, 2021 15:27
@rkuykendall rkuykendall restored the deep-update branch January 22, 2021 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants