Fix #471: Drop default value initialization at the widget level.

[n1k0]

Implements simple solution suggested by @crumblix in #471 (comment)

Deployed to gh-pages for easier testing.

r=? @crumblix

[n1k0]

Grmbl the resulting null value in cleared array items still needs fixing.

Edit: these null values are actually JSON serialization of undefined.

[n1k0]

Blocked by tdegrunt/jsonschema#206

[n1k0]

Unblocked with b9e2e6d

[n1k0]

I think we have a patch here. I've deployed its latest version to gh-pages, I'd appreciate feedback.

[n1k0]

@crumblix may I insist on having your feedback with this one? Thanks ;)

[crumblix]

Yes Grmbl indeed!

I've tried it out and thought long and hard about it.

Although initially for it, I think I'm now coming down against the array part of the changes. The main problem is that you cannot save the form for a NULL (or empty string) in the case of "A list of strings"
or in "Fixed array without buttons" if you clear "A number".

Effectively these fields act as being "mandatory even though we haven't said they are. There isn't a * next to them so the user knows they are mandatory, and quite frankly I think we should be supporting a "no value" result in these cases.

If we leave out the array changes the value comes through as "null". Which is probably closer than the alternative perhaps? It means we can test for a missing value in the formData and still allow submission. I'm not sure if in the array case we can do better than that can we? We can't remove the item or the order will be wrong, and I don't think we should error if the user hasn't entered anything. What do you think?

I'd really be interested in some more feedback for other people too.

I still like removing defaultFieldValue though in the base classes.

[n1k0]

The main problem is that you cannot save the form for a NULL (or empty string) in the case of "A list of strings"

Well, that's on purpose. A "list of strings" cannot contain a null value (as null is obviously not a string). If you really wanted a nullable type (string or null), you'd define a schema like this:

{
  type: ["string", "null"]
}

... which is unfortunately not supported by the library at this moment, though I expect most users wanting actual strings instead of mixed arrays of null and strings anyway (well, I do).

I'd really be interested in some more feedback for other people too.

Me too.

Edit: Just for the sake of clarity, in the playground clearing a string field in an array will set null as its value in the list in the reflected formData, though it's not possible to submit these data as they don't comply to the schema, which expects every members to be actual strings. Errors will be properly contextually rendered about that, so that's both informative and consistent with the spec, I can't see any problem with it. Am I missing something obvious?

[crumblix]

I can't actually disagree with you, but I still have UI concerns.

The problem is if I clear a string or clear a number (in the case of "Fixed array without buttons") I cannot submit the form.

In the case of simple lists like "A list of Strings" I could agree that erroring might be the correct course of action and even make sense to the user, but in the other cases on that page where it's representing a complex object rather than a simple list it doesn't seem to work from a UI point of view.

How about this. If we keep the change can we mark in the captions that the fields are mandatory on the "arrays as objects" cases? e.g. "A string value *" and "a boolean value *" and "additional item *" in "A List of fixed Items". (Note: We'd also need to remove the blank option on the boolean drop down, really make it mandatory).

At least that way it will make sense to people using the forms. So if you are using arrays to represent objects the individual elements are mandatory.

And if you don't want mandatory items in the array you should type your objects explicitly? Make that clear in the documentation?

[n1k0]

The problem is if I clear a string or clear a number (in the case of "Fixed array without buttons") I cannot submit the form.

Wait, what should be the right behavior for the "No add, remove and order buttons" section then? Should we just remove the string field entirely when clearing it? That would really surprising UX to me. A fixed list of strings requires every items to be an actual string, so the form behavior makes perfect sense to me.

How about this. If we keep the change can we mark in the captions that the fields are mandatory on the "arrays as objects" cases?

That makes perfect sense, I'll try to add this to the patch.

And if you don't want mandatory items in the array you should type your objects explicitly? Make that clear in the documentation?

That would certainly be a possibility, but it would feel to me like rewriting the jsonschema spec again. Why would one use jsonschema at all if to not know about its strict data structure validation capabilities? I understand that for some people *that's just a way to build forms", but with forms come data models, and with data models come types and validation, so that's fair game imho.

We'll certainly need to make that statement as clear as possible early in the docs, sure.

[crumblix]

Oh I agree we can't remove the field entirely in the array case, that would surprise me as well. No, in the case of simple lists like "No add remove buttons" the error is currently fine (although perhaps unnecessary if the widget is marked as mandatory since hopefully the mandatory constraint will kick in first and make more sense to the user).

I think the right behaviour from a UI point of view is if it acts like a mandatory field treat it as one. If I can't submit the form with an empty control effectively we only support mandatory for arrays.

If we implemented your other suggestion above:

{
  type: ["string", "null"]
}

... then that could change. But in the meantime it's mandatory, and developers need to understand if they use arrays to represent objects that's what they'll get.

[crumblix]

OK. So I've tried reverting the array changes above and putting "required={true}" against the two SchemaField declarations in ArrayField.js.

That looks to me like it fixes the problem from all angles. You even can have the nice HTML5 "required" warnings instead of the validation ones if you want.

When (or if) we support null in the schema like you suggest above then we remove those two lines setting "required" and we're good.

Thoughts?

[knilink]

About

value is now reset to undefined instead of being set to "" (empty string) as previously

, I think it only meaningful when it is a property of an object.

So, I would prefer to have this in the object field

onPropertyChange = (name) => {
    return (value, options) => {
      if(isEmpty(value)){
        const {[name]:_, ...newFormData} = formData;
      }else{
        const newFormData = {...formData, [name]:value};
      }
      this.props.onChange(newFormData, options);
    };
  };

where "isEmpty" could be customizable via registry. (I would be happy if isEmpty includes {} and []).

This also guarantee the from will alway return a non-null value such as
{"type":"string"}
it will still return an empty string rather than undefined.

[knilink]

I feel that handling the default value is the source of evil. It make both users and the form want to take control over the fromData.
Example:

class Foo extends Component {
  constructor(props){
    super(props);
    this.state = {value:undefined}
  }
  clear = ()=>{
    this.setState({value:undefined});
  }
  onChange = ({target:{value}})=>{
    this.setState({
      value:value===""?undefined:value
    });
  }
  render() {
    return <div><input
               type="text"
               defaultValue="foo"
               value={this.state.value}
               onChange={this.onChange}/>
    <button onClick={this.clear}>clear</button>
    </div>
  }
}

where the "defaultValue" is the exactly same as the default value in the schema.
In this case the react library will throw a warning

Warning: Foo contains an input of type text with both value and defaultValue props. Input elements must be either controlled or uncontrolled (specify either the value prop, or the defaultValue prop, but not both). Decide between using a controlled or uncontrolled input element and remove one of these props. More info: https://fb.me/react-controlled-components

So I believe it is necessary to differentiate "controlled" and "uncontrolled" formData.
If it is uncontrolled then the formData will be initialize(getStateFromProps) once only and preserve it in the Form component.
If it is controlled then the from read the formData directly from the props.
The rest of components, fields and widgets, are "dumb components" which shoud just display exactly what they are told to and should not preserve anything related to formData in their internal state.

[n1k0]

@crumblix I'd like you to review 53e4b51

Non-nullable array item widgets are now marked as required. I didn't remove the array mapping to mark blanked string fields as null (instead of leaving them undefined) as this is required to have the jsonschema validation library doing its job (I already commented above that part of the code in the global patch).

Edit: Deployed to gh-pages for easier QA.

[crumblix]

Yes! Moving the logic to isItemRequired is good. :)

However, I could be wrong, but I don't think we need new line at 422 in ArrayField.js. It's not currently even being unpacked in renderArrayFieldItem, and renderArrayFieldItem calls isItemRequired anyway.

Overall I think it's an elegant solution for now.

[n1k0]

However, I could be wrong, but I don't think we need new line at 422 in ArrayField.js.

Yeah that was a leftover 😊

On next green build I'm gonna land this and make a release.

[crumblix]

Excellent :)

[n1k0]

Released in v0.43.0.

[quentin-sommer]

Hi,

What's the situation for non-array string fields?

I think that resetting to the default value when clearing the field is a good practice. I think that the example that led to this (where "bazinga" was auto filled) was a specific case.

Most of the time we want string fields to have empty strings as default value, I think this is a bad decision and I'm having a hard time implementing the empty string value with this form

[n1k0]

I think this is a bad decision and I'm having a hard time implementing the empty string value with this form

Ok so please send a PR. Thank you.