gcanti / tcomb-form

Forms library for react
https://gcanti.github.io/tcomb-form
MIT License
1.16k stars 136 forks source link

How to check for form validity in `onChange`? #262

Closed Xananax closed 8 years ago

Xananax commented 8 years ago

When one sets the state in an onChange handler, it triggers a re-render and the tcomb-form never gets to update its value.

test case:

import React from 'react';
import ReactDOM from 'react-dom';
import t from 'tcomb-form'
var Form = t.form.Form;

var Person = t.struct({
  name: t.String,
  surname: t.String
});

var App = React.createClass({
  getInitialState(){
    return {valid:false}
  },
  onChange(value, path) {
    var value = this.refs.form.getValue()
    if(value){
      this.setState({valid:true});
    }else{
      this.setState({valid:false});
    }
  },

  render() {
    var valid = this.state.valid;
    return (
      <div>
        <Form ref="form"
          type={Person}
          onChange={this.onChange}
        />
        <button disabled={!valid}>{valid?'ok':'invalid'}</button>
      </div>
    );
  }

});

function render(){
    ReactDOM.render(<App/>, document.getElementById('Content'));
}

render();

Workaround: set value in the parent component's state, and pass it as a value. it seems cumbersome though to do that, when all that's needed is to check on each change if the form is valid.

Arguably, this could be fixed by changing:

https://github.com/gcanti/tcomb-form/blob/master/src/components.js#L138

const value = this.getTransformer().format(props.value)
this.setState({ value })

to

if(!Nil.is(props.value)){
   const value = this.getTransformer().format(props.value)
   this.setState({ value })
}

Another possibility (not sure):

Changing:

  onChange(fieldName, fieldValue, path, kind) {
    const value = t.mixin({}, this.state.value)
    value[fieldName] = fieldValue
    this.setState({ value }, () => {
      this.props.onChange(value, path, kind)
    })
  }

to

  onChange(fieldName, fieldValue, path, kind) {
    const value = t.mixin({}, this.state.value)
    value[fieldName] = fieldValue
    this.props.onChange(value, path, kind)
    this.setState({ value })
  }

But I'm not sure if it's a breaking change. The second most surely is.

Edit: note that I'm using getValue() here for convenience, but using var valid = (this.refs.form.validate().errors.length <= 0); yields the same results, the key being updating state (thus triggering a re-render).

Xananax commented 8 years ago

Just for anyone having the same problem, if you're wrapping the Form component, here's the minimal setup:

import React from 'react'
import t from 'tcomb-form'
const Form = t.form.Form;

export default class MyClass extends React.Component{
    constructor(props,context){
        super(props,context);
        this.state = {valid:props.valid,value:props.item && props.item.doc}
        this.onChange = this.onChange.bind(this);
        this.onSave = this.onSave.bind(this);
    }
    onSave(evt){
        evt && evt.preventDefault();
        const value = this.refs.form.getValue();
        if(value){
            //do something with the value
        }
    }
    onChange(value,path){
        const valid = !!this.refs.form.getValue();
        // or: const valid = (this.refs.form.validate().errors.length<=0)
        this.setState({valid,value});
    }
    render(){
        const {valid,value} = this.state;
        return (<form onSubmit={this.save}>
            <Form type={someType} onChange={this.onChange}/>
            <button disabled={!valid} onClick={this.save}>save</button>
        </form>)
    }
}
gcanti commented 8 years ago

Workaround: set value in the parent component's state, and pass it as a value. it seems cumbersome though to do that

Not sure I'm follow, it's not a workaround, it's how controlled components usually work in React land:

const App = React.createClass({

  getInitialState() {
    return { valid: false }
  },

  onChange(value) {
    const valid = this.refs.form.validate().isValid()
    this.setState({ valid, value })
  },

  render() {
    return (
      <form onSubmit={this.onSubmit}>
        <t.form.Form
          ref="form"
          type={Person}
          value={this.state.value}
          onChange={this.onChange}
        />
        <div className="form-group">
          <button type="submit" className="btn btn-primary">Save</button>
        </div>
      </form>
    )
  }

})

Arguably, this could be fixed by changing...

You'd loose the benefit of a controlled component, i.e. changing the value based on external business logic:

const App = React.createClass({

  getInitialState() {
    return { valid: false }
  },

  onChange(value) {
    const valid = this.refs.form.validate().isValid()

    //
    // change the value on the fly
    // business logic here...
    //

    const business = {
      name: value.name,
      surname: value.name === 'Giulio' ? 'Canti' : value.surname
    }
    this.setState({ valid, value: business })
  },

  render() {
    return (
      <form onSubmit={this.onSubmit}>
        <t.form.Form
          ref="form"
          type={Person}
          value={this.state.value}
          onChange={this.onChange}
        />
        <div className="form-group">
          <button type="submit" className="btn btn-primary">Save</button>
        </div>
      </form>
    )
  }

})
Xananax commented 8 years ago

Yes, indeed, it's not unexpected for onChange, when it exists, to have to control the value. I guess my question could be better formulated with: Is there a way to observe changes in the form without controlling it, and if yes, how?

gcanti commented 8 years ago

Is there a way to observe changes in the form without controlling it, and if yes, how?

Making t.form.Form optionally uncontrolled?

export class Form extends React.Component {

  // added
  constructor(props) {
    super(props)
    this.state = { value: props.value } // <= statefull
  }

  // added
  componentWillReceiveProps(props) {
    if (props.hasOwnProperty('value')) { // <= checking for a `value` prop
      this.setState({ value: props.value })
    }
  }

  // added
  onChange(value, path, kind) {
    this.state.value = value
    if (this.props.onChange) {
      this.props.onChange(value, path, kind)
    }
  }

  ...

  render() {

    ...

    return React.createElement(getComponent(type, options), {
      ref: 'input',
      type: type,
      options,
      value: this.state.value, // <= modified
      onChange: this.onChange.bind(this), // modified
      ctx: this.props.ctx || {
        context: this.props.context,
        uidGenerator,
        auto: 'labels',
        templates,
        i18n,
        path: []
      }
    })
  }

}

But does it worth it? Controlled components seem an established pattern and this would make t.form.Form non standard, is there a particular reason why you don't want a controlled component?

Xananax commented 8 years ago

No, I suppose it's a bit unnecessary. I just wanted to be able to observe the form for changes and disable or enable the "save" button according to form's validity without any further handling, but I suppose it's a bit of a corner case: I try to avoid state, typically, put all my state in a redux store that I can restore when the user refreshes the tab, and have only pure functions dumb components.

...But...

I need to store the form values anyway to restore them. Chalk it to lack of sleep, temporary brainmelt. Thanks for taking the time to write an answer and sorry for the bother :)

Not sure about proper etiquette...Should I close the issue myself?

gcanti commented 8 years ago

No problem :)