call props.onChange on Form component mount to set any defaults in sc…

[llamamoray]

Reasons for making this change

This fixes #1033 where schema defaults aren't being set on any model that is being updated using the form onChange prop.

Checklist

• I'm updating documentation
  • I've checked the rendering of the Markdown text I've added
  • If I'm adding a new section, I've updated the Table of Content
• 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

[llamamoray]

Hi @glasserc @n1k0

Would you be able to take a look at this PR?

[glasserc]

Thanks for the great write-up! (And sorry for taking so long to get to this.)

My intuition is that if we use a default anywhere in the formData, then the data has changed, but if the default isn't used (because e.g. there's a value for that property already) then the data has not changed. I'd prefer to see onChange only called when the data actually changes, but I'm willing to accept something like this if figuring that out is difficult.

Looking at the code, I see that Form calls getStateFromProps in componentWillReceiveProps. Does that mean we need to add a call to onChange somewhere in that part of the lifecycle too?

[llamamoray]

Thanks for taking a look at this.

I see what you're saying about the onChange callback only being called when the data has actually changed. I can do a deep compare of the the state vs the props formData in the componentDidMount and only call onChange in this case:

  componentDidMount() {
    if (this.props.onChange
      && !deepEquals(this.state.formData, this.props.formData)) {
      this.props.onChange(this.state);
    }
  }

Would you be happy with that?

To your second point about the other lifecycle events, I think in theory onChange could be called in componentWillReceiveProps if and only if the the schema has changed. Something like this:

// Form.js
  componentWillReceiveProps(nextProps) {
    const nextState = this.getStateFromProps(nextProps);
    this.setState(nextState);
    if (!deepEquals(this.props.schema, nextProps.schema)) {
      if (this.props.onChange) {
        this.props.onChange(nextState);
      }
    }
  }

The problem with this is that unless the formData prop is cleared on the form before the schema prop is changed, it will have no effect on the internal formData state of the rjsf form. This is because getStateFromProps calls getDefaultFormState which effectively takes the current formData and merges in the default values (which wouldn't occur as they've already been set). I guess the question is: is it worth doing a deep compare of the schema on every prop change when with the current implementation it doesn't set the default in any case?

[glasserc]

Regarding componentWillReceiveProps, I was thinking in terms of the caller of rjsf changing the formData or schema props out from under us. If the schema adds a field with a default, or adds a default to an existing field, wouldn't that involve setting the default? But then I realized I didn't know if componentWillReceiveProps is the appropriate lifecycle method or not, and when I went to the React docs, and I see that it's being renamed to UNSAFE_componentWillReceiveProps. Oh boy. Well, maybe it's better not to rely on such a method. I get the feeling that we need to really rethink all this deepEquals stuff that we're doing all over the place, but that's way outside the scope of this PR. So let's drop this point for the time being.

One suggestion in the docs is that we update state in the component constructor. Maybe we should do the calls to onChange in the constructor -- what do you think? Otherwise, yes, doing deepEqual like you propose is fine.

[llamamoray]

Moving the componentDidMount logic into the contructor makes sense 👍 - I'll go ahead and do this with the deep equals check.

I think the correct method to use to replace componentWillReceiveProps is probably componentDidUpdate. This would involve doing a deep compare of all those props and also moving the library from react 15 to React 16.x.

For now componentWillReceiveProps is the best way to hook into the lifecycle event for changes to the schema or form data props, so I propose checking for changes to those inside there and firing onChange if required.

A bigger piece of work will be to upgrade the library to a newer version of react - a lot of the components may need to be changed. For now they are supporting the legacy lifecycle methods until version 17 albeit with the UNSAFE_ prefix which we could run this codemod to update the library.

[glasserc]

Great, thanks!

[graingert]

I don't think this is the correct solution - I think rjsf should expose a function to clean formData and crash if it receives unclean formData

[ml242]

Getting an onChange error and endless loop now with my form -- it seems like updating state in the componentWillReceiveProps is the problem. Investigating further now.

[llamamoray]

@graingert what do you mean by "clean data"? The problem I was seeing was if you had a form where you were using the onChange prop to store form state outside of rjsf and the schema contained some defaults, then your "external" form state would not reflect the defaults that rjsf sets in it's internal form state on initialisation.

[llamamoray]

@ml242 how did your investigations go? Any chance you could create a jsfiddle demonstrating the problem?

[graingert]

@llamamoray currently rjsf calls onChange with cleaned data immediately during construction. This shouldn't happen