Add NullField

[faissalMT]

Reasons for making this change

The JSON Schema type 'null' is currently validated and inferred as a type but shows an error when trying to render. This corrects the issue such that null fields will render nothing, allowing for things like additional help text or use as a temporary placeholder field.

#1232

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
• I'm adding or updating code
  • I've added and/or updated tests
  • I've updated docs if needed
  • I've run npm run cs-format on my branch to conform my code to prettier coding style
• I'm adding a new feature
  • I've updated the playground with an example use of the feature

[epicfaace]

@faissalMT , thanks for the PR. In #1232, you mention that null fields should render the title only and not the input box -- but this PR makes null fields render nothing entirely. Was this intentional?

Additionally, I'm wondering how this change plays with the changes done in #1213. If this PR makes it such that "null" fields render nothing, then will our current handling of nullable types be unintuitive?

(#1213 makes a field with type ["string", "null"] still render the input box for a string. If "null" fields actually render nothing, could one intuitively expect a field with type ["string", "null"] to actually render a OneOf widget with either an input box or nothing in it?)

Maybe this isn't really a concern though so I'd like to hear your thoughts @faissalMT @warrenseymour

[wms]

@epicfaace I've not taken the time to clone this locally and experiment, but as far as I can tell, from a code perspective, this shouldn't clash with what I did in #1213, since that does a reasonably extensive check that the field type is [<T>, "null"] or ["null, <T>] and then simply passes the type on as <T>. So I think in the case presented here, type: "null" should be handled as @faissalMT intends.

From a UX perspective, however, I agree that explicitly adding a Null widget may lead to confusion and I would be inclined to push this sort of decision onto the consumer of RJSF - if they want to explicitly enable a Null widget, it should be on them to opt into by bringing their own Widget, or at least explicitly enabling that option if it is built-in.

Finally, trying to infer/envisage a legitimate use-case for this I looked at a commit that @faissalMT cited in #1232. It looks as if he's just trying to render a piece of inert content in the form. Would a uiSchema entry using a custom widget be a reasonable way of solving that issue?

[faissalMT]

@epicfaace

but this PR makes null fields render nothing entirely. Was this intentional?

RJSF itself is taking care of handling the rendering of those things (as per the playground example I added), so there was no need for me to implement it explicitly.

@warrenseymour

I agree that explicitly adding a Null widget may lead to confusion and I would be inclined to push this sort of decision onto the consumer of RJSF

I would expect any native JSON schema type to be renderable by RJSF, if the user doesn't like the handling they can always override it. I think this makes the most sense and is significantly better than an error.

Would a uiSchema entry using a custom widget be a reasonable way of solving that issue?

In that project we have a certain level of preprocessing that generates a uiSchema, however we must provide it with a non-null type at the moment or RJSF will refuse to render our custom widget. This also works well for creating placeholder fields.

[epicfaace]

@faissalMT thanks, I didn't realize that it was rendering the title/description as well.

It makes sense how rjsf should have some kind of default widget for null types. However, my primary concern is this: when you make a field such as helpText with type: null, then you would expect helpText: null to be in the formData. However, in this case, there is no key for helpText in the formData.

What if we make helpText required? Then we get stuck with an "is a required property" error, which is only resolved when you actually manually set helpText: null in the formData.

Perhaps the solution for this is to make sure that a NullWidget calls the onChange handler with null, so that helpText: null is actually set in the formData. I'd be ready to merge this PR if we decide it makes sense to do that.

I don't know if, as @warrenseymour noted, this entirely fits the use case to "render a piece of inert content in the form". Do we want to clutter our formData by adding in extra null keys when we just want to add in additional content in the form? Perhaps the real solution fo this is in the uiSchema.

[faissalMT]

@epicfaace

Perhaps the solution for this is to make sure that a NullWidget calls the onChange handler with null

I like this solution and have committed it. An alternative could also simply be to use a default value, but that seems more tacky to me.

Do we want to clutter our formData by adding in extra null keys when we just want to add in additional content in the form? Perhaps the real solution fo this is in the uiSchema.

I feel like modifying uiSchema to work with non-existent fields as if they are existing fields could quickly get ugly. The uiSchema may serve to further expand on the details of this content (such as use of a custom widget etc), but items in the uiSchema should always correspond to a real field or I believe some fundamental assumptions this project has made are broken.

[epicfaace]

Is there a reason why you did this in componentWillMount instead of componentDidMount?

[faissalMT]

I just forgot it was deprecated

[epicfaace]

Perhaps it would be better to call onChange only if props.value === undefined?

[faissalMT]

Agreed, that behaviour might come in handy.

[faissalMT]

Is there anything blocking a review?

[epicfaace]

@faissalMT there's just one thing, thanks for the reminder. I was hesitant to merge this because unlike other widgets, this is the only widget that automatically mutates the formData without any user input (by calling onChange on load). I'm not sure if that could potentially cause problems, or if it is okay -- what do you think?

[faissalMT]

@epicfaace The system doesn't really have anything that would distinguish between actors and this is a really simple manipulation so I don't think there'll be any issues.

[epicfaace]

Okay, thanks. What do you mean by "the system doesn't really have anything that would distinguish between actors"?

[faissalMT]

@epicfaace I mean that nothing outside the widgets themselves in RJSF has implemented any assumptions about what kind of actor is entering the data (although they may have been programmed with that assumption), there would be nothing preventing a widget being set by fetched data for example.

Our project currently implements a number of automatically generated fields and it really isn't an issue for individual widgets acting on their own data. RJSF only cares about which widget is entering the data for which form field, and we're not breaking the assumption that a widget will only change the data of its field.

[epicfaace]

Sounds good, thanks for the explanation.

[epicfaace]

Should NullField have proptypes of the other fields, such as in:

https://github.com/mozilla-services/react-jsonschema-form/blob/ab0be5ec42939fc699a60a3779d743ea595ad496/src/components/fields/StringField.js#L60

[faissalMT]

@epicfaace I didn't realise we had a central proptypes object for fields, I've changed it now.

[epicfaace]

Thanks! Looks good.

[tomasgvivo]

@glasserc When will this changes be released?

[ghost]

@epicfaace Will this PR eventually be merged or will the issue with null as default value be resolved in an entirely different way?

[epicfaace]

@rmdevos this PR has already been merged -- however, it only addresses fields with type: null, not necessarily fields with default: null.