Suggestion: in onChange return "undefined" instead of "empty string" or "empty array"

[crumblix]

Prerequisites

• I have read the documentation;
• In the case of a bug report, I understand that providing a SSCCE example is tremendously useful to the maintainers.

Description

When there is no data for checkbox or text input ... return undefined instead of empty string or empty array to be consistent with other field types.

From what I can gather based on recent commits this year. The correct behaviour for NULL or clearing of an input field is to return "undefined". This will cause the field to be removed from the JSON result data. This is good in two ways.

1. It means that an empty string won't raise an error for being an invalid integer (for example).
2. The mandatory check kicks in.

Let's say I have a Name string field which is required. Now normally (say for strings) an empty string is not a problem. As the HTML "required" tag will cause the browser to prompt the user to input a string.

But now let's assume I have another error as well in my form ... could be anything, but for now assume it's a field whose widget (for whatever reason) doesn't support the "required" tag. For me this was a custom checkbox widget to support a NULL value. That's fine but when the error list is now displayed it tells me that my boolean value is required, but doesn't mention the empty Name string field.

Why? Because an empty string is still a string. From a validation point of view you HAVE a string. Basically we're saved in most cases from this by virtue of the fact the "required" tag is kicking in.

I've noticed this on "string" fields and checkbox arrays ....

Yes ... mandatory on a checkbox array. Although the current code doesn't support it, with this fix if you pass in "undefined" instead of empty array, and then also pass through the "required" parameter to Widget in Arrayfield it works.

You won't be able to use the "required" HTML tag for the checkbox list (that would mean you have to select every item), but the validation routine then picks up the error when you click submit.

I'll be submitting another bug (fix) for the required tag that would enable this shortly, but this would lay the groundwork for that as well.

Expected behavior

My suggestion is that to be consistent with other controls, when the string field is empty string "" it should return "undefined" like other controls. Likewise an empty array [], should never be returned either in onClick, rather it should also return "undefined".

This makes it consistent, and allows the validation to work as well.

Actual behavior

String return an empty string "". Which is still a string for validation purposes.
Arrays (like from checkboxes) return an empty array []" Which is still an array for validation purposes.

Version

Latest.

Not sure of the proper protocol for submitting patches, but I have forked the project here:

https://github.com/crumblix/react-jsonschema-form

Made a meal of the commits ... lost an underscore in the first commit, but the combination of the two commits below in that branch is basically what I am suggesting. Sorry it's not just on the one commit, but I can refork and clean it up later if you like the general idea.

Commit: 2aad37f [2aad37f]
Commit: a901626 [a901626]

(Diffs were here ... I've removed them since the formatting didn't work)

If you don't like it though I won't be offended :)

I'll submit the other changes for consideration shortly.

[n1k0]

My suggestion is that to be consistent with other controls, when the string field is empty string "" it should return "undefined" like other controls

An empty string should be considered as a valid string value if the jsonschema allows that; undefined means the field has no value attached to it at all. Both are very valid use cases to me.

What we need is a way to "reset" a given field value, eg. with pressing a "clear" button next to the widget, so it's attached value switches from "" to undefined and reflects the change into formData.

This is currently discussed in #402.

[crumblix]

OK. I'll move the discussion there. It's the age old battle of zero length string versus NULL.

I get that's a preference thing.

I'd prefer to leave this open as well for now though as it also is to do with arrays. Is that OK?

Thanks!

[n1k0]

Sure, we'll close according to the outcome of the ongoing discussions.

[crumblix]

Sounds like an excellent plan :)

Not sure if I'm following correct protocol with these bug fixes/suggestions. If you'd prefer I do it in a different way let me know.

Thanks!

[n1k0]

Not sure if I'm following correct protocol with these bug fixes/suggestions. If you'd prefer I do it in a different way let me know.

Ideally pull requests are a great way to start good conversations, but I understand this may be overkill or frustrating sometimes. For now let's try to align on the strategy in the comments of #402.

[n1k0]

It seems people are agreeing on this behavior and want to see it implemented in the lib, see ongoing dicussion in #402.

I'd like to make them happy and land this along #420, would you be willing to post your modifications as a single consistent PR so it's easier to review and discuss the implementation? Thanks!

[crumblix]

As agreed, I'll merge my changes from #410 into #420 and take it from there :)

[crumblix]

This has now all been wrapped up in a new PR #442.

[n1k0]

#442 has landed and has been released in v0.42.0.