fix: backbone set patch in ill defined state after exceptions in handler

If an exception occurs during set() in backbone-patch.ts this._changing
is left true. This means that next calls to set() will skip the
trigger('change', ...) code.

This fix will leave the backbonejs model in a better defined state.

FYI: This bug is present upstream in backbonejs as well.

Author: maartenbreddels

packages/base/src/backbone-patch.ts

@@ -76,58 +76,65 @@ export function set(
   const changes = [];
   const changing = this._changing;
   this._changing = true;
-
-  if (!changing) {
-    // EDIT: changed to use object spread instead of _.clone
-    this._previousAttributes = { ...this.attributes };
-    this.changed = {};
-  }
-
-  const current = this.attributes;
-  const changed = this.changed;
-  const prev = this._previousAttributes;
-
-  // For each `set` attribute, update or delete the current value.
-  for (const attr in attrs) {
-    val = attrs[attr];
-    // EDIT: the following two lines use our isEqual instead of _.isEqual
-    if (!utils.isEqual(current[attr], val)) {
-      changes.push(attr);
-    }
-    if (!utils.isEqual(prev[attr], val)) {
-      changed[attr] = val;
-    } else {
-      delete changed[attr];
+  try {
+    if (!changing) {
+      // EDIT: changed to use object spread instead of _.clone
+      this._previousAttributes = { ...this.attributes };
+      this.changed = {};
     }
-    unset ? delete current[attr] : (current[attr] = val);
-  }
 
-  // Update the `id`.
-  this.id = this.get(this.idAttribute);
-
-  // Trigger all relevant attribute changes.
-  if (!silent) {
-    if (changes.length) {
-      this._pending = options;
+    const current = this.attributes;
+    const changed = this.changed;
+    const prev = this._previousAttributes;
+
+    // For each `set` attribute, update or delete the current value.
+    for (const attr in attrs) {
+      val = attrs[attr];
+      // EDIT: the following two lines use our isEqual instead of _.isEqual
+      if (!utils.isEqual(current[attr], val)) {
+        changes.push(attr);
+      }
+      if (!utils.isEqual(prev[attr], val)) {
+        changed[attr] = val;
+      } else {
+        delete changed[attr];
+      }
+      unset ? delete current[attr] : (current[attr] = val);
     }
-    for (let i = 0; i < changes.length; i++) {
-      this.trigger('change:' + changes[i], this, current[changes[i]], options);
+
+    // Update the `id`.
+    this.id = this.get(this.idAttribute);
+
+    // Trigger all relevant attribute changes.
+    if (!silent) {
+      if (changes.length) {
+        this._pending = options;
+      }
+      for (let i = 0; i < changes.length; i++) {
+        this.trigger(
+          'change:' + changes[i],
+          this,
+          current[changes[i]],
+          options
+        );
+      }
     }
-  }
 
-  // You might be wondering why there's a `while` loop here. Changes can
-  // be recursively nested within `"change"` events.
-  if (changing) {
-    return this;
-  }
-  if (!silent) {
-    while (this._pending) {
-      options = this._pending;
-      this._pending = false;
-      this.trigger('change', this, options);
+    // You might be wondering why there's a `while` loop here. Changes can
+    // be recursively nested within `"change"` events.
+    if (changing) {
+      return this;
+    }
+    if (!silent) {
+      while (this._pending) {
+        options = this._pending;
+        this._pending = false;
+        this.trigger('change', this, options);
+      }
     }
+  } finally {
+    this._pending = false;
+    this._changing = false;
   }
-  this._pending = false;
-  this._changing = false;
   return this;
 }

packages/base/test/src/widget_test.ts

@@ -536,6 +536,33 @@ describe('WidgetModel', function () {
       expect(changeA).to.be.calledBefore(change);
       expect(change).to.be.calledWith(this.widget);
     });
+    it('triggers change events after exception', async function () {
+      const changeA = sinon.spy(function changeA() {
+        return;
+      });
+      const changeB = sinon.spy(function changeB() {
+        throw 'error';
+      });
+      const change = sinon.spy(function change() {
+        return;
+      });
+      this.widget.on('change:a', changeA);
+      this.widget.on('change:b', changeB);
+      this.widget.on('change', change);
+      // first we trigger a set that causes an exception
+      try {
+        this.widget.set('b', 42);
+      } catch {
+        // empty
+      }
+      expect(change).to.not.be.called;
+      // from now on this test is similar to 'triggers change events'
+      this.widget.set('a', 100);
+      expect(changeA).to.be.calledOnce;
+      expect(changeA).to.be.calledWith(this.widget, 100);
+      expect(changeA).to.be.calledBefore(change);
+      expect(change).to.be.calledWith(this.widget);
+    });
     it('handles multiple values to set', function () {
       expect(this.widget.get('a')).to.be.undefined;
       expect(this.widget.get('b')).to.be.undefined;