facebook / react

The library for web and native user interfaces.
https://react.dev
MIT License
228.57k stars 46.79k forks source link

Wierd functionality when try to display complex array #6292

Closed stavenko closed 8 years ago

stavenko commented 8 years ago

Hello.

I got one strange error-like thing. I got array of commands in very special component, eg:

class ScenarioForm extends Component{
  constructor(props){ 
    super(props);
    this.state= {scenario : props.scenario}
  }
  componentWillReceiveProps(props){
    this.setState({scenario : props.scenario});
  }

  commandsUpdated(commandsArray){
    this.state.scenario.commands=commandsArray;
    this.forceUpdate();
  }

  render(){
    if(!this.state.scenario) {
      return <span>nothing</span>;
    }
    return <div>
      //skipped
      <div className='form-group'>
        <div className={'col-xs-12'}> 
          <div className={[ cs.commandsContainer].join(' ')}>
            {
              this.state.scenario.commands.map((c,ix)=>{
                return <Command 
                  key={ c.id } 
                  command={c} 
                  ix={ix}
                  array={this.state.scenario.commands}
                  parent={this}
                />
                    })
            }
          </div>
        </div>
      </div>
      // skipped...
  }
}

Each command is a:

export class Command extends Component{
  constructor(props){
    super(props);
  }

  componentWillReceiveProps(props){
    this.props = props
    this.forceUpdate();
  }

  render(){
    let { children, array, parent, command,ix } = this.props;

    let index = command.id;
    let CurrentCommand = Commands[command.type];

    return <div className={s.command} >
      <div className={s.commandHead}>
        <span>#</span> <input className={s.ixInput} value={index} onChange=    {this.changeOrder.bind(this)}/>
        <span>{ix}</span>
        <div className={s.commandButtons}>
          <div className={[s.button,s.clone].join(' ')} onClick={this.clone.bind(this)}>
            <span className='glyphicon glyphicon-link'/>
          </div>
          <div className={[s.button,s.remove].join(' ')} onClick={this.remove.bind(this)}>
            <span className='glyphicon glyphicon-remove'/>
          </div>
        </div>
      </div>
      <div className={s.commandBody}>
        <CurrentCommand {...{command, array, parent}} />
      </div>
    </div>
  }

  changeOrder(e){
    let {array, command} = this.props;
        let index = command.id;

    let newIndex = parseInt(e.currentTarget.value);
    let otherCommandWithSameIndexId = array
      .map((c,ix)=>c.id==newIndex?ix:-1)
      .filter(x=>x>=0);

    let otherId; 
    if(!otherCommandWithSameIndexId.length) otherId = array.length - 1;
    else otherId = otherCommandWithSameIndexId[0];

    let other = array[otherId];
    let me    = array[index];
    let id_ = other.id;
    other.id = me.id;
    me.id = id_;
   array.sort((a,b)=>a.id - b.id);
   this.commandsUpdated(array);
  }

  remove(){
    let {array, command} = this.props;
    let index = command.id;

    let me = array.splice(index,1)[0];
    for(let ix = 0; ix < array.length; ++ix){
      array[ix].id =ix;
    }
    this.commandsUpdated(JSON.parse(JSON.stringify(array)));
  }
  clone(){
    let {array} = this.props;
    let me = array[this.props.index];
    let newMe = JSON.parse(JSON.stringify(me));
    newMe.id +=1;
    array.splice(me.id,0, newMe);
    for(let ix = newMe.id+1; ix < array.length; ++ix){
      array[ix].id +=1;
    }
    this.commandsUpdated();
  }
  commandsUpdated(...args){
    let {parent} = this.props;
    parent.commandsUpdated(...args);
  }
}

And CurrentCommand is a:

class SleepCommand extends Component{
  constructor(props){
    super(props)
  }
  componentWillReceiveProps(props){
    this.props=props;
    this.forceUpdate();
  }
  render(){
    let {index, array, parent, command} = this.props;
    return <div className={s.sleepCommand}> 
      <span> Sleep for </span>
          <input className={s.sleepInput}  value={command.sleepFor} onChange={(e)=>{
        let v = parseInt(e.currentTarget.value);
        v = isNaN(v)?0:v;
        command.sleepFor = v;
        parent.commandsUpdated(array);
      }}/> 
    <span>ms</span>
    </div>
  }
}

or

class UnknownCommand extends Component{
  constructor(props){
    console.log("PROPS", props);
    super(props)
  }
  componentWillReceiveProps(props){
    this.props=props;
    this.forceUpdate();
  }

  render(){
    let {command, array, parent} = this.props
    console.info("Unknow command", command, array, parent);
    let index = command.id;
    return <div> 
      <div>
        <label>Select command type</label>
      </div>
      <select onChange={this.selectType.bind(this)}>
        <option>-----</option>
        {CommandTypes.map((c,ix)=><option key={ix} value={ix}>{c}</option>)}
      </select>
    </div>

  }

  selectType(e){
    let {command, array, parent} = this.props
    let index = command.id;
    console.info("Unknow command st", command, array, parent);
    let id = e.currentTarget.value;
    let type = CommandTypes[id];
    console.log('change', array);
    array[index].type=type;
    parent.commandsUpdated(array);
  }
}

There is a remove button in command root. The main idea is to remove certain element in array and then redraw it. Sometimes i can see, that array is spliced correctly - it identity stored correctly. But sometimes, wrong element is deleted with exception:

Uncaught Invariant Violation: findComponentRoot(..., .0.0.1.0.4.1.1.0.0.$2.0.4.0): Unable to find element. 

There's explanation in this exception:

 usually due to forgetting a <tbody> when using tables, nesting tags like <form>, <p>, or <a>, or using non-SVG elements in an <svg> parent. Try inspecting the child nodes of the element with React ID ``. 

I have no any of this elements, I have putted keys to commands, and it seems to me, that react doesn't handle elements with key property correctly.

When i try too look at available elements, it was exists before remove function called.

I have no idea, why incorrect dom element is removed(since removed element left on page), and when I have correct array when i console.log it's items in ScenarioForm::render method.

stavenko commented 8 years ago

After a whole day of looking for mistery thing in my code, I have discovered the main reason of this bug:

In some undefined moment, one nasty chrome extension prepended two div elements and appended one element to container, from which react should have removed the staff. (Those my english madskills).

Disabling this extention solved my problem and made my day completed. No more work on this particular problem is required, but, I'd rather suggested to all you, respectful developers to implement some small functionality, that under the hood, filters non-react elements out to hell. Or tells to some noob like me, that page state changes in undefined way.

gaearon commented 8 years ago

Yeah, this is something we are considering. You can help, too: https://github.com/facebook/react/issues/3218

On an unrelated note, I some anti-patterns in your code. While this not relevant to this issue I figure I’d mention them:

1) Deriving state based on props is usually an anti-pattern. It’s better to move state management to the parent component and provide a callback to the child as a prop. 2) Using forceUpdate() is also an anti-pattern. React will automatically re-render your component when you call setState() in it or its parent. 3) Assinging something directly to anything inside this.state is an anti-pattern. I suggest you to create a new object based on the old one, and call setState() to update the relevant part of the state with a reference to the new object. Treating state as immutable lets you add powerful performance optimizations later because it is possible to compare which objects have changed by comparing their references with the previous values. 4) There is never a reason to assign this.props. Props are immutable. React will update the reference to this.props after componentWillReceiveProps() exits. Don’t do this manually.

I recommend you to read through Thinking in React which covers top-down data flow and state ownership. While React provides escape hatches that you are using, they are absolutely not the primary way to build React apps, and you might have problems later on unless you got back to these topics and get familiar with them.

I hope that this is helpful. Cheers!

stavenko commented 8 years ago

3) Assinging something directly to anything inside this.state is an anti-pattern. I would really appreciate some advices to achieve this with a single this.setState instruction. I had an issue with complex forms, when I had to set properties with objectPath.set(this.state, 'some.sophisticated.input.data.path', e.currentTarget.value); I had to to this with let s = this.state; <modify s>; this.setState(s), which seems too me same as <modify this.state>; this.setState(this.state). Actually, I started to use <modify this.state>; this.forceUpdate();

4) There is never a reason to assign this.props. Whoa. Since I have debug-ed react a lot to find reason of this issue, now I see abilities how too prevent state where it's not needed and immutable props, this knowledge will affect a lot of my previously done code.

gaearon commented 8 years ago

You can use something like React immutability helpers for working plain objects or Immutable.js which provides API for deep updates.

In general if your state is very complicated you might want to consider moving its handling out of the component with a library like http://redux.js.org (disclaimer: I wrote it). It lets you describe changes in state as pure functions that call each other, and “reducer composition” pattern used there corresponds to the deep updates you want to implement.

Finally, even if you mutate the state directly, you still don’t need to reassign the props. The correct way to go with mutation is to perform it in the component that owns the state, and to call setState in it. Then the change will propagate down to any children—you don’t need to do anything with props to make it happen.