Bug related to cacheing/memoizing of old component states

[ezmiller]

Hi, I'm encountering some trouble with react-cursor. The bug that I'm encountering is in a relatively simple component where I add/remove items from a list. The bug manifests itself in many ways. The simplest is that if I add an item to the list and the try to remove it, the item will not be removed.

When I attempted to debug this by looking into what react-cursor is doing, it appears that the memoize function calculates the hashed key for the new state, in which there are no items in the list, and then checks to see if the hashed key is already in the cache variable. Strangely, the hashed key for the component state with items in the list is the same as the new state without the list, and so the memoize returns the previously cached component that still has the item in the list.

I've been trying to debug this myself, since I'd like to contribute to your project, but have hit a bit of a wall. From what I've found it seems as though there could be a problem in the hash function such that it returns the same value for two different strings. Any suggestions?

[dustingetz]

What version of react?

[dustingetz]

Can you paste the source code of the list component with add/remove?

[ezmiller]

0.13.1

[ezmiller]

Here it is:

/**
 * StartDiscussion
 */
/*jslint browser:true*/
(function() {
  'use strict';

  var React = require('react');
  var Search = require('./Search.jsx');
  var Source = require('./Source.jsx');
  var Cursor = require('react-cursor').Cursor;
  var ImmutableOptimizations = require('react-cursor').ImmutableOptimizations;
  var Actions = require('../actions/Actions.js');
  var DiscussionStore = require('../stores/DiscussionStore.js');

  var SourceSelector = React.createClass({

    propTypes: {
      selected: React.PropTypes.instanceOf(Cursor).isRequired
    },

    getDefaultProps: function () {
      return {
        selected: null  
      };
    },

    getInitialState: function () {
      return {
        addingSource: false  
      };
    },

    componentWillUpdate: function (nextProps, nextState) {
      console.log('SourceSelector::componentWillUpdate():', {nextState:nextState, nextProps:nextProps});
    },

    componentWillReceiveProps: function (nextProps) {
      console.log('SourceSelector::componentWillReceiveProps():', {nextProps:nextProps});
    },

    handleAddSourceClick: function() {
      this.setState({addingSource: true});
    },

    handleRemoveSourceClick: function(e, source) {
      var newSelected;

      newSelected = this.props.selected.value.filter(function(v) {
        return v.id !== source.id;
      });

      if (newSelected.length === 0) newSelected = [];

      this.props.selected.set(newSelected);
      // this.props.selected.set([]);
    },

    handleSearchFormClick: function(source) {
      var selected = this.props.selected.value;
      selected.push(source);
      this.props.selected.set(selected);
      this.setState({addingSource: false});
    },

    render: function() {
      var selected, selectedSourceItems;

      selected = this.props.selected.value;

      console.log('SourceSelector::render() called');

      selectedSourceItems = selected !== null && selected.length > 0 ? selected.map(function(item, i) {
        return <Source key={i} source={item} onClick={this.handleRemoveSourceClick} />;
      }.bind(this)) : null;

      return (
        <div className="source-selector">
          {selectedSourceItems}
          <div className="add-source" onClick={this.handleAddSourceClick}>Add Source</div>
          <Search resultsBtnLabel="Add" handler={this.handleSearchFormClick} />
        </div>
      );
    }

  });

  var StartDiscussion = React.createClass({

    // mixins: [ImmutableOptimizations(['sources'])],

    propTypes: {
      user: React.PropTypes.instanceOf(Cursor).isRequired
    },

    getInitialState: function () {
      return {
        title: '',
        prompt: '',
        private: true,
        visible: false,
        sources: []
      };
    },

    componentDidMount: function () {
      DiscussionStore.addEventListener('sync', this.onSync);
      DiscussionStore.addEventListener('error', this.onSyncError);
    },

    componentWillUnmount: function () {
      DiscussionStore.removeEventListener('error', this.onSync);
      DiscussionStore.removeEventListener('error', this.onSyncError);
    },

    componentWillUpdate: function (nextProps, nextState) {
      console.log('StartDiscussion::componentWillUpdate():', {nextState:nextState, nextProps:nextProps});
    },

    componentWillReceiveProps: function (nextProps) {
      console.log('StartDiscussion::componentWillReceiveProps():', {nextProps:nextProps});
    },

    handleSubmit: function(e) {
      var data;

      e.preventDefault();

      data = this.state;
      data.owner = this.props.user.value.id;
      console.log('submitting new discussion: ', data);

      Actions.createDiscussion(data);
    },

    handleChange: function(e) {
      var target, nextState = {};
      target = e.target.name; 
      nextState[target] = e.target.type === 'checkbox' ? e.target.checked : e.target.value;
      this.setState(nextState);
    },

    render: function() {
      var cursor = Cursor.build(this);

      console.log('StartDiscussion::render()', this.state);

      return (
        <div className="start-discussion eight columns offset-by-two">
          <div className="form-wrap">
            <form id="start-discussion" onSubmit={this.handleSubmit}>
              <h4>Start a Discussion</h4>
              <div>
                <input 
                  type="text" name="title" 
                  placeholder="Discussion Title" 
                  onChange={this.handleChange} 
                  value={this.state.title} />
              </div>
              <div>
                <input 
                  type="text" 
                  name="prompt" 
                  placeholder="Topic or Prompt" 
                  onChange={this.handleChange} 
                  value={this.state.prompt} />
              </div>
              <div>
                <input 
                  type="checkbox" 
                  id="private" 
                  name="private" 
                  onChange={this.handleChange} 
                  checked={this.state.private} />
                <label htmlFor="private">Private Discussion</label>
              </div>
              <div>
                <input 
                  type="checkbox" 
                  id="visible" 
                  name="visible" 
                  onChange={this.handleChange} 
                  checked={this.state.visible} />
                <label htmlFor="visible">Visible to Public  </label>
              </div>
              <div>
                <SourceSelector selected={cursor.refine('sources')} />
              </div>
              <button id="cancel" name="cancel">Cancel</button>
              <button id="start" name="start">Start</button>
            </form>
          </div>
        </div>
      );
    },

    onSync: function(discussion) {
      console.log('StartDiscussion::onSync() called:', discussion);
    },

    onSyncError: function(err) {
      console.log('StartDiscussion::onSyncError() called:', error);
    }

  });

  module.exports = StartDiscussion;

}());

[ezmiller]

Here is some console output that shows where it is happening. The highlighted hashed key is the one that should be different I think. In the first highlighted instance, the "result" value that is highlighted represents the hash key value when the state data contains for "sources" contains an empty array. Then in the other instances, from inside the memoize function, the lookup there finds a hash in the cache with that key, but the object that that key corresponds to contains state data where "sources" holds an array with the source that has just been removed.

[meta-meta]

handleRemoveSourceClick: function(e, source) {
      var newSelected;

      newSelected = this.props.selected.value.filter(function(v) {
        return v.id !== source.id;
      });

      if (newSelected.length === 0) newSelected = [];

      this.props.selected.set(newSelected);
      // this.props.selected.set([]);
    },

this is called in the click handler passed to Source here:

selectedSourceItems = selected !== null && selected.length > 0 ? selected.map(function(item, i) {
        return <Source key={i} source={item} onClick={this.handleRemoveSourceClick} />;
      }.bind(this)) : null;

What does Source.jsx look like?

[ezmiller]

Here is Source.jsx:

/**
 * Source
 */
(function() {
    'use strict';

    var React = require('react');

    var Source = React.createClass({

        getDefaultProps: function () {
        return {
        source: null  
        };
        },

        handleClick: function(e) {
            e.preventDefault();
            // publish('/app/transitionTo', ['/source/'+this.props.source.get('id')]);
            this.props.onClick(e, this.props.source);
        },

        render: function() {
            var source, authors;

            source = this.props.source;

            authors = !source ? null : source.authors.map(function(author,key) {
                return <span key={key} className="author">{author.firstName} {author.lastName}</span>;
            });

            return(
                <article className="source book eight columns" onClick={this.handleClick}>
                    <div className="cover">
                        <img className="image" src={source.imageLinks.thumbnail} alt="Book Cover" />
                    </div>
                    <div className="info">
                        <div className="meta">
                            <h3 className="title">{source.title}</h3>
                            <h5 className="subtitle">{source.subtitle}</h5>
                            <span className="authors">by {authors}</span>
                        </div>
                    </div>
                </article>
            );
        }

    });

    module.exports = Source;
}());

[ezmiller]

I think the problem may not be with the hash method, but the other way around: somehow it seems that the cached object associated with the original hashed key for the empty component state is being changed to contain an item in the source list. You can see that here -- note the same hashed key and yet in the first instance the source array is empty, later it's got one. So when react-cursor goes to retrieve the item is has been changed and now includes a source item:

[dustingetz]

We have a pretty good guess to what the issue is. We have some commits in a branch that I'm going to ask you to test, I'm working on getting them ready right now.

[dustingetz]

Just pushed new changes to master. They aren't published to any package managers yet. The cursor updater methods now can take an object or updateFunction, same api as react 13's cmp.setState. The cursor.pendingValue api has been removed as it has been broken in react 13 - though it looks like you weren't using it. I actually don't think these new commits are going to fix your issue though.

[dustingetz]

I'd like to see this in a debugger, would you be able to create a minimum standalone demo?

[ezmiller]

This should work for a demo. Clone and then follow instructions in the Readme. https://github.com/ezmiller/react-cursor-debug

[lijunle]

This line of code does the bad thing. It modifies the original list.

Solution: modify it to:

this.props.selected.push(source);

---

Long story

The following talk why the item is not removed - and why the hash indicate cursor instance has one item in the list.

1. Initialize. everything is good. In the hash, [hash-0] -> cursor-0 -> empty list
2. Add an item. Because you are modifying the original list. The hash will be updated to [hash-0] -> cursor-0 -> [adding-item-0]. However, because you are not using shouldComponetUpdate helper, React will re-render to update DOM. This re-render action build up another hash, [hash-1] -> cursor-1 -> [adding-item-0].
3. Remove the adding item. Everything goes good - the item in filtered out in the list, the cursor.set works great with an empty list, the setState function is called internally by Cursor.update. Because setState is called, React is going to re-render the component, it goes to this line to build cursor (and get hash). Currently, the this.state hold an empty list, you will get the initial hash value [hash-0], which points to cursor-0 -> [adding-item-0]. (Yes, not empty list because it is modified in step 2.)
4. Lastly, you get a modified list with your adding item. Your remove action goes into trouble.

[ezmiller]

Thank you @lijunle. That was it. I changed the code in that handler to this:

handleSearchFormClick: function(source) {
  this.props.selected.push(Array(source));
  this.setState({addingSource: false});
}

And now it works well.

I wonder if we should issue a warning to the user or prevent all together this way of accessing/modifying the cursor, since it is not apparent until one has looked into react-cursor's code that the cache is being modified in this way. Or is that even possible? Is there any way to issue a warning when someone modifies the cursor value directly in the way that I did?

[lijunle]

I am sorry to say, that is impossible to detect such modifications in react-cursor. The only thing can do is to clarify it in README.

[dustingetz]

I wonder if there is some creative way to detect it, ignoring performance concerns, as this seems a common stumbling point. For example, we can run an updater function (new api) on a mock object and check the mock for mutations, before running on the actual react state. This could be turned off in production. Just a wild thought, it might turn out to be impossible for all cases.

[ezmiller]

I agree that it seems like a common pitfall. And it's quite a challenge to debug. Making it clear that this is a major thing to avoid, with some explanation of the consequences, would be a good place to start, but I think that a good warning in development would be very good. It would also be consistent with the kind of warnings that the React library itself gives in development.

[dustingetz]

Does React actually detect out-of-band mutations now?

[ezmiller]

Actually, not sure what you mean by out-of-band. I just meant that React tends, in developoment mode, to give guidance about what not to do. So when I develop in React, I tend to watch such warnings closely.

On this issue, I'm interested in helping out here in any way you can. I'm looking to get some open source experience. So please use me in any way you see fit. I'd happily try to code this up, although I'd probably need to discuss with you a bit about how this might be accomplished.

[lijunle]

React does a shadow copy on the first level object.

E.g., (JSFiddle)

var a = {
    b: 10
};

var b = React.addons.update(a, { $apply: function (obj) {
    obj.b = 20;
    return obj;
} });

// a.b -> 10
// b.b -> 20

But... (JSFiddle)

var a = {
    b: {
        c: 10
    }
};

var b = React.addons.update(a, { $apply: function (obj) {
    obj.b.c = 20;
    return obj;
} });

// a.b.c = b.b.c -> 20

---

It means, React.addons.update helper does not handle mutation fully. If we want to avoid this situation, one way is to use Immutable.js. But I don't think you are going to do that.

IMO, just clarify that in README, and teach user to use the right way is OK.

[ezmiller]

Where did this end up being left off?

[dustingetz]

Where did this end up being left off?

If someone codes up an elegant way to detect illegal mutations, I will merge it.

[ezmiller]

Okay, when I get a chance, I'll give it a shot. It's an interesting problem. Any tips/ideas to get me started? This is a new kind of challenge for me. Not sure where to begin in fact.

[dustingetz]

Well, I'm not entirely convinced that it's possible to detect out-of-band mutations. I haven't thought it all the way through.

One idea, is that the cursor update methods (set, push, etc.) can hang on to a reference to the final value right before they pass it to react setState. Then in each subsequent invocation to a cursor update method, we can recursively check that the current cursor.value is reference-equal to the previous value at all nodes of the tree, before performing the update. This would have performance implications.

[dustingetz]

Closing this issue, opened #54 to track this new task.