fix: adds check for null model to componentWillReceiveProps (#21)

* fix: adds check for null model to componentWillReceiveProps

There are some cases where a Backbone model passed through CBR
might become undefined. (Consider the case where a view relies on
a secondary model as a "decorator", and the use of that "decorator"
model is toggled on or off.) Today, when a model becomes undefined,
CBR throws a TypeError in ComponentWillReceiveProps:

Uncaught TypeError: Cannot read property 'on' of undefined

This commit adds a check for null or undefined models before trying
to set event listeners in componentWillReceiveProps.

* Removes listeners from models which become undefined

* DRYs removeEventListener code

* Reference model directly in componentWillReceiveProps

* Checks this.context.models for models becoming undefined

Previously, we only checked this.props.models for models becoming
undefined in componentWillReceiveProps. This commit also checks
this.context.models, but only if a matching model was not found in
this.props.models -- because of the Object.assign in setModels,
if there are two models with the same mapKey, we prefer the one
in props.

* Adds test for model becoming undefined in componentWillReceiveProps

* Micro-optimize and clean componentWillReceiveProps

* Refactor undefined model test per review

* Adds testing for initial model listener bindings

* Adds tests: createListener and removeListener are called for decorator model

* Updates test titles and comments per review

Author: logandavis

lib/connect-backbone-to-react.js

@@ -107,6 +107,12 @@ module.exports = function connectBackboneToReact(
         });
       }
 
+      removeEventListener(modelName, model) {
+        getEventNames(modelName).forEach(name => {
+          model.off(name, this.createNewProps, this);
+        });
+      }
+
       createNewProps() {
         // Bail out if our component has been unmounted.
         // The only case where this flag is encountered is when this component
@@ -138,11 +144,28 @@ module.exports = function connectBackboneToReact(
         // Bind event listeners for each model that changed.
         Object.keys(this.models).forEach(mapKey => {
           const model = this.models[mapKey];
-          if ((this.props.models && this.props.models[mapKey] === this.models[mapKey]) ||
-            (this.context.models && this.context.models[mapKey] === this.models[mapKey])) {
-            return; // Did not change.
+
+          // Retrieve old versions of the model from props and context for comparison.
+          const propsModel = this.props.models ? this.props.models[mapKey] : undefined;
+          const contextModel = this.context.models ? this.context.models[mapKey] : undefined;
+
+          // Do not attempt to create event listeners on an undefined model.
+          if (!model) {
+            // Instead, if it was previously defined, remove the old listeners.
+            if (propsModel) {
+              this.removeEventListener(mapKey, propsModel);
+              // If a model with the matching mapKey exists in both props and context,
+              // we only remove listeners from the one in props. We do this because
+              // only the one in props is actually used in this.models, per the
+              // Object.assign in setModel.
+            } else if (contextModel) {
+              this.removeEventListener(mapKey, contextModel);
+            }
+            return;
           }
 
+          if ((propsModel === model) || (contextModel === model)) return; // Did not change.
+
           this.createEventListener(mapKey, model);
         });
       }
@@ -156,10 +179,7 @@ module.exports = function connectBackboneToReact(
           const model = this.models[mapKey];
           // Do not attempt to remove event listeners on an undefined model.
           if (!model) return;
-
-          getEventNames(mapKey).forEach(name => {
-            model.off(name, this.createNewProps, this);
-          });
+          this.removeEventListener(mapKey, model);
         });
 
         this.hasBeenUnmounted = true;

test/connect-backbone-to-react.test.js

@@ -594,6 +594,78 @@ describe('connectBackboneToReact', function() {
     });
   });
 
+  describe('when passed props change to include', function() {
+    let ConnectedTest;
+    let setStateSpy;
+    let createListenerSpy;
+    let removeListenerSpy;
+
+    beforeEach(function() {
+      ConnectedTest = connectBackboneToReact(mapModelsToProps)(TestComponent);
+      wrapper = mount(<ConnectedTest models={modelsMap} />);
+
+      setStateSpy = sandbox.spy(ConnectedTest.prototype, 'setState');
+      createListenerSpy = sandbox.spy(ConnectedTest.prototype, 'createEventListener');
+      removeListenerSpy = sandbox.spy(ConnectedTest.prototype, 'removeEventListener');
+
+      const decoratorUserModel = new UserModel({
+        name: 'Robert',
+        age: '30',
+        hungry: false,
+      });
+
+      const initialModelsMap = {
+        user: userModel,
+        coll: userCollection,
+        decorator: decoratorUserModel,
+      };
+
+      wrapper.setProps({ models: initialModelsMap });
+    });
+
+    afterEach(function() {
+      wrapper.unmount();
+    });
+
+    it('calls setState once', function() {
+      assert.equal(setStateSpy.callCount, 1);
+    });
+
+    it('calls createEventListener once due to decoratorUserModel being added as a model', function() {
+      assert.equal(createListenerSpy.callCount, 1);
+      assert.equal(createListenerSpy.firstCall.args[0], 'decorator');
+    });
+
+    it('does not call removeEventListener', function() {
+      assert.equal(removeListenerSpy.callCount, 0);
+    });
+
+    describe('an undefined model', function() {
+      beforeEach(function() {
+        const newModelsMap = {
+          user: userModel,
+          coll: userCollection,
+          decorator: undefined,
+        };
+
+        wrapper.setProps({ models: newModelsMap });
+      });
+
+      it('calls setState again', function() {
+        assert.equal(setStateSpy.callCount, 2);
+      });
+
+      it('does not call createEventListener again', function() {
+        assert.equal(createListenerSpy.callCount, 1);
+      });
+
+      it('calls removeEventListener once for decoratorUserModel', function() {
+        assert.equal(removeListenerSpy.callCount, 1);
+        assert.equal(removeListenerSpy.firstCall.args[0], 'decorator');
+      });
+    });
+  });
+
   describe('when unmounted in an event listener and subscribed to "all" event', function() {
     // To add more color, "all" event handlers are triggered after individual event handlers.
     // That is to say, if you trigger "foo" the sequence of event handlers called is: