Merge pull request #1848 from bookshelf/rg-previous-attributes

Fix inconsistent previous attributes

docs/scripts/linenumber.js

@@ -1,25 +1,25 @@
 /*global document */
 (function() {
-    var source = document.getElementsByClassName('prettyprint source linenums');
-    var i = 0;
-    var lineNumber = 0;
-    var lineId;
-    var lines;
-    var totalLines;
-    var anchorHash;
+  var source = document.getElementsByClassName('prettyprint source linenums');
+  var i = 0;
+  var lineNumber = 0;
+  var lineId;
+  var lines;
+  var totalLines;
+  var anchorHash;
 
-    if (source && source[0]) {
-        anchorHash = document.location.hash.substring(1);
-        lines = source[0].getElementsByTagName('li');
-        totalLines = lines.length;
+  if (source && source[0]) {
+    anchorHash = document.location.hash.substring(1);
+    lines = source[0].getElementsByTagName('li');
+    totalLines = lines.length;
 
-        for (; i < totalLines; i++) {
-            lineNumber++;
-            lineId = 'line' + lineNumber;
-            lines[i].id = lineId;
-            if (lineId === anchorHash) {
-                lines[i].className += ' selected';
-            }
-        }
+    for (; i < totalLines; i++) {
+      lineNumber++;
+      lineId = 'line' + lineNumber;
+      lines[i].id = lineId;
+      if (lineId === anchorHash) {
+        lines[i].className += ' selected';
+      }
     }
+  }
 })();

docs/scripts/main.js

@@ -1,13 +1,13 @@
 (function() {
-  var navbarHeight = document.querySelector('.main-navbar').offsetHeight
+  var navbarHeight = document.querySelector('.main-navbar').offsetHeight;
 
   function scrollWithOffset() {
-    var targetElement = document.querySelector(':target')
-    window.scroll({top: targetElement.offsetTop - navbarHeight})
+    var targetElement = document.querySelector(':target');
+    window.scroll({top: targetElement.offsetTop - navbarHeight});
   }
 
-  window.addEventListener('hashchange', scrollWithOffset, false)
-  window.addEventListener('DOMContentLoaded', scrollWithOffset, false)
+  window.addEventListener('hashchange', scrollWithOffset, false);
+  window.addEventListener('DOMContentLoaded', scrollWithOffset, false);
 
-  document.getElementById('show-menu').checked = false
-}())
+  document.getElementById('show-menu').checked = false;
+})();

docs/scripts/prettify/lang-css.js

@@ -1,2 +1,23 @@
-PR.registerLangHandler(PR.createSimpleLexer([["pln",/^[\t\n\f\r ]+/,null," \t\r\n"]],[["str",/^"(?:[^\n\f\r"\\]|\\(?:\r\n?|\n|\f)|\\[\S\s])*"/,null],["str",/^'(?:[^\n\f\r'\\]|\\(?:\r\n?|\n|\f)|\\[\S\s])*'/,null],["lang-css-str",/^url\(([^"')]*)\)/i],["kwd",/^(?:url|rgb|!important|@import|@page|@media|@charset|inherit)(?=[^\w-]|$)/i,null],["lang-css-kw",/^(-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*)\s*:/i],["com",/^\/\*[^*]*\*+(?:[^*/][^*]*\*+)*\//],["com",
-/^(?:<\!--|--\>)/],["lit",/^(?:\d+|\d*\.\d+)(?:%|[a-z]+)?/i],["lit",/^#[\da-f]{3,6}/i],["pln",/^-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*/i],["pun",/^[^\s\w"']+/]]),["css"]);PR.registerLangHandler(PR.createSimpleLexer([],[["kwd",/^-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*/i]]),["css-kw"]);PR.registerLangHandler(PR.createSimpleLexer([],[["str",/^[^"')]+/]]),["css-str"]);
+PR.registerLangHandler(
+  PR.createSimpleLexer(
+    [['pln', /^[\t\n\f\r ]+/, null, ' \t\r\n']],
+    [
+      ['str', /^"(?:[^\n\f\r"\\]|\\(?:\r\n?|\n|\f)|\\[\S\s])*"/, null],
+      ['str', /^'(?:[^\n\f\r'\\]|\\(?:\r\n?|\n|\f)|\\[\S\s])*'/, null],
+      ['lang-css-str', /^url\(([^"')]*)\)/i],
+      ['kwd', /^(?:url|rgb|!important|@import|@page|@media|@charset|inherit)(?=[^\w-]|$)/i, null],
+      ['lang-css-kw', /^(-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*)\s*:/i],
+      ['com', /^\/\*[^*]*\*+(?:[^*/][^*]*\*+)*\//],
+      ['com', /^(?:<\!--|--\>)/],
+      ['lit', /^(?:\d+|\d*\.\d+)(?:%|[a-z]+)?/i],
+      ['lit', /^#[\da-f]{3,6}/i],
+      ['pln', /^-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*/i],
+      ['pun', /^[^\s\w"']+/]
+    ]
+  ),
+  ['css']
+);
+PR.registerLangHandler(PR.createSimpleLexer([], [['kwd', /^-?(?:[_a-z]|\\[\da-f]+ ?)(?:[\w-]|\\\\[\da-f]+ ?)*/i]]), [
+  'css-kw'
+]);
+PR.registerLangHandler(PR.createSimpleLexer([], [['str', /^[^"')]+/]]), ['css-str']);

lib/base/model.js

@@ -24,6 +24,7 @@ function ModelBase(attributes, options) {
   let attrs = attributes || {};
   options = options || {};
   this.attributes = Object.create(null);
+  this._previousAttributes = {};
   this._reset();
   this.relations = {};
   this.cid = _.uniqueId('c');
@@ -294,7 +295,7 @@ ModelBase.prototype.set = function(key, val, options) {
   // Extract attributes and options.
   const unset = options.unset;
   const current = this.attributes;
-  const prev = this._previousAttributes;
+  const prev = this.previousAttributes();
 
   // Check for changes of `id`.
   if (this.idAttribute in attrs) this.id = attrs[this.idAttribute];
@@ -661,15 +662,27 @@ ModelBase.prototype.timestamp = function(options) {
  * @method
  * @description
  *
- * Returns true if any {@link Model#attributes attribute} attribute has changed
- * since the last {@link Model#fetch fetch}, {@link Model#save save}, or {@link
- * Model#destroy destroy}. If an attribute is passed, returns true only if that
- * specific attribute has changed.
+ * Returns `true` if any {@link Model#attributes attribute} has changed since
+ * the last {@link Model#fetch fetch} or {@link Model#save save}. If an
+ * attribute name is passed as argument, returns `true` only if that specific
+ * attribute has changed.
  *
- * @param {string=} attribute
+ * Note that even if an attribute is changed by using the {@link Model#set set}
+ * method, but the new value is exactly the same as the existing one, the
+ * attribute is not considered *changed*.
+ *
+ * @example
+ * Author.forge({id: 1}).fetch().then(function(author) {
+ *   author.hasChanged() // false
+ *   author.set('name', 'Bob')
+ *   author.hasChanged('name') // true
+ * })
+ *
+ * @param {string=} attribute A specific attribute to check for changes.
  * @returns {Boolean}
- * `true` if any attribute has changed. Or, if `attribute` was specified, true
- * if it has changed.
+ *   `true` if any attribute has changed, `false` otherwise. Alternatively, if
+ *   the `attribute` argument was specified, checks if that particular
+ *   attribute has changed.
  */
 ModelBase.prototype.hasChanged = function(attr) {
   if (attr == null) return !_.isEmpty(this.changed);
@@ -680,11 +693,31 @@ ModelBase.prototype.hasChanged = function(attr) {
  * @method
  * @description
  *
- * Returns the this previous value of a changed {@link Model#attributes
- * attribute}, or `undefined` if one had not been specified previously.
+ * Returns the value of an attribute like it was before the last change. A
+ * change is usually done with the {@link Model#set set} method, but it can
+ * also be done with the {@link Model#save save} method. This is useful for
+ * getting back the original attribute value after it's been changed. It can
+ * also be used to get the original value after a model has been saved to the
+ * database or destroyed.
+ *
+ * In case you want to get the previous value of all attributes at once you
+ * should use the {@link Model#previousAttributes previousAttributes} method.
+ *
+ * Note that this will return `undefined` if the model hasn't been fetched,
+ * saved, destroyed or eager loaded. However, in case one of these operations
+ * did take place, it will return the current value if an attribute hasn't
+ * changed. If you want to check if an attribute has changed see the
+ * {@link Model#hasChanged hasChanged} method.
+ *
+ * @example
+ * Author.forge({id: 1}).fetch().then(function(author) {
+ *   author.get('name') // Alice
+ *   author.set('name', 'Bob')
+ *   author.previous('name') // 'Alice'
+ * })
  *
- * @param {string} attribute The attribute to check
- * @returns {mixed} The previous value
+ * @param {string} attribute The attribute to check.
+ * @returns {mixed} The previous value.
  */
 ModelBase.prototype.previous = function(attribute) {
   return this._previousAttributes[attribute];
@@ -694,29 +727,53 @@ ModelBase.prototype.previous = function(attribute) {
  * @method
  * @description
  *
- * Return a copy of the {@link Model model}'s previous attributes from the
- * model's last {@link Model#fetch fetch}, {@link Model#save save}, or {@link
- * Model#destroy destroy}. Useful for getting a diff between versions of a
- * model, or getting back to a valid state after an error occurs.
+ * Returns a copy of the {@link Model model}'s attributes like they were before
+ * the last change. A change is usually done with the {@link Model#set set}
+ * method, but it can also be done with the {@link Model#save save} method.
+ * This is mostly useful for getting a diff of the model's attributes after
+ * changing some of them. It can also be used to get the previous state of a
+ * model after it has been saved to the database or destroyed.
+ *
+ * In case you want to get the previous value of a single attribute you should
+ * use the {@link Model#previous previous} method.
+ *
+ * Note that this will return an empty object if no changes have been made to
+ * the model and it hasn't been fetched, saved or eager loaded.
+ *
+ * @example
+ * Author.forge({id: 1}).fetch().then(function(author) {
+ *   author.get('name') // Alice
+ *   author.set('name', 'Bob')
+ *   author.previousAttributes() // {id: 1, name: 'Alice'}
+ * })
+ *
+ * Author.forge({id: 1}).fetch().then(function(author) {
+ *   author.get('name') // Alice
+ *   return author.save({name: 'Bob'})
+ * }).then(function(author) {
+ *   author.get('name') // Bob
+ *   author.previousAttributes() // {id: 1, name: 'Alice'}
+ * })
  *
- * @returns {Object} The attributes as they were before the last change.
+ * @returns {Object}
+ *   The attributes as they were before the last change, or an empty object in
+ *   case the model data hasn't been fetched yet.
  */
 ModelBase.prototype.previousAttributes = function() {
-  return _.clone(this._previousAttributes);
+  return _.clone(this._previousAttributes) || {};
 };
 
 /**
  * @method
  * @private
  * @description
  *
- * Resets the `_previousAttributes` and `changed` hash for the model.
- * Typically called after a `sync` action (save, fetch, delete) -
+ * Resets the `changed` hash for the model. Typically called after a `sync`
+ * action (save, fetch, destroy).
  *
  * @returns {Model} This model.
  */
 ModelBase.prototype._reset = function() {
-  this._previousAttributes = _.cloneDeep(this.attributes);
   this.changed = Object.create(null);
   return this;
 };

lib/collection.js

@@ -441,8 +441,11 @@ const BookshelfCollection = (module.exports = CollectionBase.extend(
         remove: options.remove,
         silent: true,
         parse: true
-      }).invokeMap('formatTimestamps');
-      this.invokeMap('_reset');
+      }).invokeMap(function() {
+        this.formatTimestamps();
+        this._reset();
+        this._previousAttributes = _.cloneDeep(this.attributes);
+      });
 
       if (relatedData && relatedData.isJoined()) {
         relatedData.parsePivot(this.models);

lib/model.js

@@ -1137,10 +1137,6 @@ const BookshelfModel = ModelBase.extend(
                 }
               }
 
-              // In case we need to reference the `previousAttributes` for the this
-              // in the following event handlers.
-              options.previousAttributes = this._previousAttributes;
-
               this._reset();
 
               /**
@@ -1254,6 +1250,8 @@ const BookshelfModel = ModelBase.extend(
           if (options.require !== false && affectedRows === 0) {
             throw new this.constructor.NoRowsDeletedError('No Rows Deleted');
           }
+
+          this._previousAttributes = _.clone(this.attributes);
           this.clear();
 
           /**
@@ -1454,6 +1452,7 @@ const BookshelfModel = ModelBase.extend(
       this.set(this.parse(response[0]), {silent: true})
         .formatTimestamps()
         ._reset();
+      this._previousAttributes = _.cloneDeep(this.attributes);
 
       if (relatedData && relatedData.isJoined()) {
         relatedData.parsePivot([this]);

lib/relation.js

@@ -418,7 +418,8 @@ const Relation = RelationBase.extend({
     // its parsed attributes
     related.map((model) => {
       model.attributes = model.parse(model.attributes);
-      model.formatTimestamps()._reset();
+      model.formatTimestamps()._previousAttributes = _.cloneDeep(model.attributes);
+      model._reset();
     });
 
     return related;

scripts/jsdoc.config.json

@@ -7,19 +7,15 @@
     "includePattern": ".+\\.js(doc)?$",
     "excludePattern": "(^|\\/|\\\\)_"
   },
-  "plugins": [
-    "node_modules/jsdoc/plugins/markdown"
-  ],
+  "plugins": ["node_modules/jsdoc/plugins/markdown"],
   "templates": {
     "cleverLinks": true,
     "monospaceLinks": false,
     "default": {
       "outputSourceFiles": true,
       "showInheritedFrom": false,
       "staticFiles": {
-        "include": [
-          "./.jsdoc-temp"
-        ]
+        "include": ["./.jsdoc-temp"]
       }
     }
   },
@@ -34,11 +30,6 @@
     "title": "Bookshelf.js",
     "mainPageTitle": "Home",
     "tutorialsTitle": "Guides",
-    "whitelist": [
-      "Bookshelf",
-      "Model",
-      "Collection",
-      "Events"
-    ]
+    "whitelist": ["Bookshelf", "Model", "Collection", "Events"]
   }
 }

test/integration/json.js

@@ -53,14 +53,15 @@ module.exports = function(bookshelf) {
       return Command.forge({id: 0})
         .fetch()
         .then(function(command) {
-          const newTarget = {
-            x: 7,
-            y: 13
-          };
-          const updatedInfo = command.get('info');
+          const newTarget = {x: 7, y: 13};
+          const originalInfo = command.get('info');
+          const updatedInfo = _.cloneDeep(originalInfo);
           updatedInfo.target = newTarget;
+
           command.set('info', updatedInfo);
+
           expect(command.get('info')).to.not.deep.eql(command.previous('info'));
+          expect(command.previous('info')).to.deep.equal(originalInfo);
         });
     });