From d44286e287d749438e010cc286b6be0facb4ecbc Mon Sep 17 00:00:00 2001 From: shebson Date: Mon, 6 Jan 2014 14:01:03 -0800 Subject: [PATCH 1/3] Use collectionName for ModelStore key when modelName isn't set --- shared/base/view.js | 4 +-- shared/fetcher.js | 16 +++++---- shared/modelUtils.js | 16 ++++++--- shared/store/collection_store.js | 2 +- shared/store/model_store.js | 38 +++++++++++--------- test/shared/store/model_store.test.js | 51 ++++++++++++++++++++++++--- 6 files changed, 94 insertions(+), 33 deletions(-) diff --git a/shared/base/view.js b/shared/base/view.js index 2cb035a7..72976a9d 100644 --- a/shared/base/view.js +++ b/shared/base/view.js @@ -62,12 +62,12 @@ module.exports = BaseView = Backbone.View.extend({ parse: true }); } - options.model_name = options.model_name || this.app.modelUtils.modelName(options.model.constructor); + options.model_name = options.model_name || this.app.modelUtils.modelOrCollectionName(options.model.constructor); options.model_id = options.model.id; } if (options.collection != null) { - options.collection_name = options.collection_name || this.app.modelUtils.modelName(options.collection.constructor); + options.collection_name = options.collection_name || this.app.modelUtils.modelOrCollectionName(options.collection.constructor); options.collection_params = options.collection.params; } diff --git a/shared/fetcher.js b/shared/fetcher.js index 4dea019a..99380c70 100644 --- a/shared/fetcher.js +++ b/shared/fetcher.js @@ -227,7 +227,7 @@ Fetcher.prototype.fetchFromApi = function(spec, callback) { body = resp.body; resp.body = typeof body === 'string' ? body.slice(0, 150) : body; respOutput = JSON.stringify(resp); - err = new Error("ERROR fetching model '" + fetcher.modelUtils.modelName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput); + err = new Error("ERROR fetching model '" + fetcher.modelUtils.modelOrCollectionName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput); err.status = resp.status; err.body = body; callback(err); @@ -237,12 +237,16 @@ Fetcher.prototype.fetchFromApi = function(spec, callback) { Fetcher.prototype.retrieveModelsForCollectionName = function(collectionName, modelIds) { var modelName = this.modelUtils.getModelNameForCollectionName(collectionName); - return this.retrieveModels(modelName, modelIds); + if (modelName) { + return this.retrieveModels(modelName, modelIds); + } else { + return this.retrieveModels(collectionName, modelIds); + } }; -Fetcher.prototype.retrieveModels = function(modelName, modelIds) { +Fetcher.prototype.retrieveModels = function(modelOrCollectionName, modelIds) { return modelIds.map(function(id) { - return this.modelStore.get(modelName, id); + return this.modelStore.get(modelOrCollectionName, id); }, this); }; @@ -253,7 +257,7 @@ Fetcher.prototype.summarize = function(modelOrCollection) { if (this.modelUtils.isCollection(modelOrCollection)) { idAttribute = modelOrCollection.model.prototype.idAttribute; summary = { - collection: this.modelUtils.modelName(modelOrCollection.constructor), + collection: this.modelUtils.modelOrCollectionName(modelOrCollection.constructor), ids: modelOrCollection.pluck(idAttribute), params: modelOrCollection.params, meta: modelOrCollection.meta @@ -261,7 +265,7 @@ Fetcher.prototype.summarize = function(modelOrCollection) { } else if (this.modelUtils.isModel(modelOrCollection)) { idAttribute = modelOrCollection.idAttribute; summary = { - model: this.modelUtils.modelName(modelOrCollection.constructor), + model: this.modelUtils.modelOrCollectionName(modelOrCollection.constructor), id: modelOrCollection.get(idAttribute) }; } diff --git a/shared/modelUtils.js b/shared/modelUtils.js index 58a3ccb0..3544a19c 100644 --- a/shared/modelUtils.js +++ b/shared/modelUtils.js @@ -20,7 +20,7 @@ function ModelUtils(entryPath) { this._classMap = {}; } -ModelUtils.prototype.getModel = function(path, attrs, options, callback) { +ModelUtils.prototype.getModel = function(path, attrs, options, callback, fallbackToBaseModel) { var Model; attrs = attrs || {}; options = options || {}; @@ -29,7 +29,15 @@ ModelUtils.prototype.getModel = function(path, attrs, options, callback) { callback(new Model(attrs, options)); }); } else { - Model = this.getModelConstructor(path); + try { + Model = this.getModelConstructor(path) + } catch (e) { + if (e.code === 'MODULE_NOT_FOUND' && fallbackToBaseModel) { + Model = BaseModel; + } else { + throw e; + } + } return new Model(attrs, options); } }; @@ -87,7 +95,7 @@ ModelUtils.prototype.isCollection = function(obj) { ModelUtils.prototype.getModelNameForCollectionName = function(collectionName) { var Collection; Collection = this.getCollectionConstructor(collectionName); - return this.modelName(Collection.prototype.model); + return this.modelOrCollectionName(Collection.prototype.model); }; ModelUtils.uppercaseRe = /([A-Z])/g; @@ -121,7 +129,7 @@ ModelUtils.prototype.underscorize = function(name) { * -> "" * MyClass.id = "MyClass" */ -ModelUtils.prototype.modelName = function(modelOrCollectionClass) { +ModelUtils.prototype.modelOrCollectionName = function(modelOrCollectionClass) { return this.underscorize(modelOrCollectionClass.id || modelOrCollectionClass.name); }; diff --git a/shared/store/collection_store.js b/shared/store/collection_store.js index a47853f1..dbfe36c2 100644 --- a/shared/store/collection_store.js +++ b/shared/store/collection_store.js @@ -16,7 +16,7 @@ CollectionStore.prototype.constructor = CollectionStore; CollectionStore.prototype.set = function(collection, params) { var data, idAttribute, key; params = params || collection.params; - key = this._getStoreKey(this.modelUtils.modelName(collection.constructor), params); + key = this._getStoreKey(this.modelUtils.modelOrCollectionName(collection.constructor), params); idAttribute = collection.model.prototype.idAttribute; data = { ids: collection.pluck(idAttribute), diff --git a/shared/store/model_store.js b/shared/store/model_store.js index b141df7d..84b00001 100644 --- a/shared/store/model_store.js +++ b/shared/store/model_store.js @@ -14,46 +14,52 @@ ModelStore.prototype = Object.create(Super.prototype); ModelStore.prototype.constructor = ModelStore; ModelStore.prototype.set = function(model) { - var existingAttrs, id, key, modelName, newAttrs; + var existingAttrs, id, key, modelOrCollectionName, newAttrs, constructor; id = model.get(model.idAttribute); - modelName = this.modelUtils.modelName(model.constructor); - if (modelName == null) { - throw new Error('Undefined modelName for model'); + modelOrCollectionName = this.modelUtils.modelOrCollectionName(model.constructor); + if (!modelOrCollectionName && model.collection) { + modelOrCollectionName = this.modelUtils.modelOrCollectionName(model.collection.constructor); } - key = this._getModelStoreKey(modelName, id); + /** + * If the model is not named and not part of a named collection, + * fall back to an empty string to preserve existing behavior. + */ + modelOrCollectionName = modelOrCollectionName || ''; + key = this._getModelStoreKey(modelOrCollectionName, id); /** * We want to merge the model attrs with whatever is already * present in the store. */ - existingAttrs = this.get(modelName, id) || {}; + existingAttrs = this.get(modelOrCollectionName, id) || {}; newAttrs = _.extend({}, existingAttrs, model.toJSON()); return Super.prototype.set.call(this, key, newAttrs, null); }; -ModelStore.prototype.get = function(modelName, id, returnModelInstance) { +ModelStore.prototype.get = function(modelOrCollectionName, id, returnModelInstance) { var key, modelData; if (returnModelInstance == null) { returnModelInstance = false; } - key = this._getModelStoreKey(modelName, id); + key = this._getModelStoreKey(modelOrCollectionName, id); + modelData = Super.prototype.get.call(this, key); if (modelData) { if (returnModelInstance) { - return this.modelUtils.getModel(modelName, modelData, { + return this.modelUtils.getModel(modelOrCollectionName, modelData, { app: this.app - }); + }, null, true); } else { return modelData; } } }; -ModelStore.prototype.find = function(modelName, params) { +ModelStore.prototype.find = function(modelOrCollectionName, params) { var prefix, foundCachedObject, _this, data, foundCachedObjectKey; - prefix = this._formatKey(this._keyPrefix(modelName)); + prefix = this._formatKey(this._keyPrefix(modelOrCollectionName)); _this = this; // find the cached object that has attributes which are a subset of the params foundCachedObject = _.find(this.cache, function(cacheObject, key) { @@ -88,10 +94,10 @@ function isObjectSubset(potentialSubset, objectToTest) { }); } -ModelStore.prototype._keyPrefix = function(modelName) { - return this.modelUtils.underscorize(modelName); +ModelStore.prototype._keyPrefix = function(modelOrCollectionName) { + return this.modelUtils.underscorize(modelOrCollectionName); } -ModelStore.prototype._getModelStoreKey = function(modelName, id) { - return this._keyPrefix(modelName) + ":" + id; +ModelStore.prototype._getModelStoreKey = function(modelOrCollectionName, id) { + return this._keyPrefix(modelOrCollectionName) + ":" + id; } diff --git a/test/shared/store/model_store.test.js b/test/shared/store/model_store.test.js index 662327df..78140725 100644 --- a/test/shared/store/model_store.test.js +++ b/test/shared/store/model_store.test.js @@ -6,16 +6,22 @@ var util = require('util'), ModelUtils = require('../../../shared/modelUtils'), modelUtils = new ModelUtils(), AddClassMapping = require('../../helpers/add_class_mapping'), - addClassMapping = new AddClassMapping(modelUtils); + addClassMapping = new AddClassMapping(modelUtils), + BaseCollection = require('../../../shared/base/collection'); function MyModel() { MyModel.super_.apply(this, arguments); } util.inherits(MyModel, BaseModel); +function MyCollection() { + MyCollection.super_.apply(this, arguments); +} +util.inherits(MyCollection, BaseCollection); + function App() {} -addClassMapping.add(modelUtils.modelName(MyModel), MyModel); +addClassMapping.add(modelUtils.modelOrCollectionName(MyModel), MyModel); describe('ModelStore', function() { beforeEach(function() { @@ -56,7 +62,7 @@ describe('ModelStore', function() { model = new MyCustomModel(modelAttrs); this.store.set(model); - result = this.store.get(modelUtils.modelName(MyCustomModel), modelAttrs.login); + result = this.store.get(modelUtils.modelOrCollectionName(MyCustomModel), modelAttrs.login); result.should.eql(modelAttrs); }); @@ -100,7 +106,7 @@ describe('ModelStore', function() { } util.inherits(MySecondModel, BaseModel); - addClassMapping.add(modelUtils.modelName(MySecondModel), MySecondModel); + addClassMapping.add(modelUtils.modelOrCollectionName(MySecondModel), MySecondModel); it('should find a model on custom attributes', function(){ var model, modelAttrs, result; @@ -126,4 +132,41 @@ describe('ModelStore', function() { should.equal(result, undefined); }); }); + + it("should support storing models without an id if they are in a collection", function() { + var collection, model, collectionAttrs, modelAttrs, resultModel; + collectionAttrs = { + id: 'my_collection' + }; + collection = new MyCollection(collectionAttrs); + modelAttrs = { + id : 1, + foo : 'bar' + }; + model = new BaseModel(modelAttrs); + model.collection = collection; + this.store.set(model); + resultModel = this.store.get('my_collection', 1); + resultModel.should.eql(modelAttrs); + }); + + it("should return and instance of BaseModel if a model doesn't have an id and is in a collection", function() { + var collection, model, collectionAttrs, modelAttrs, resultModel; + collectionAttrs = { + id: 'my_collection' + }; + collection = new MyCollection(collectionAttrs); + modelAttrs = { + id : 1, + foo : 'bar' + }; + model = new BaseModel(modelAttrs); + model.collection = collection; + this.store.set(model); + resultModel = this.store.get('my_collection', 1, true); + resultModel.should.be.an.instanceOf(BaseModel); + resultModel.toJSON().should.eql(modelAttrs); + resultModel.app.should.eql(this.app); + }); + }); From 894572eca95c0e5c3b4625b96e629e679f67f50f Mon Sep 17 00:00:00 2001 From: shebson Date: Tue, 18 Feb 2014 16:45:14 -0800 Subject: [PATCH 2/3] Renamed getModelOrCollectionName and handle module not found in client --- shared/base/view.js | 4 ++-- shared/fetcher.js | 6 ++--- shared/modelUtils.js | 6 ++--- shared/store/collection_store.js | 2 +- shared/store/model_store.js | 32 +++++++++++++-------------- test/shared/store/model_store.test.js | 6 ++--- 6 files changed, 28 insertions(+), 28 deletions(-) diff --git a/shared/base/view.js b/shared/base/view.js index 72976a9d..d889f3e6 100644 --- a/shared/base/view.js +++ b/shared/base/view.js @@ -62,12 +62,12 @@ module.exports = BaseView = Backbone.View.extend({ parse: true }); } - options.model_name = options.model_name || this.app.modelUtils.modelOrCollectionName(options.model.constructor); + options.model_name = options.model_name || this.app.modelUtils.resourceName(options.model.constructor); options.model_id = options.model.id; } if (options.collection != null) { - options.collection_name = options.collection_name || this.app.modelUtils.modelOrCollectionName(options.collection.constructor); + options.collection_name = options.collection_name || this.app.modelUtils.resourceName(options.collection.constructor); options.collection_params = options.collection.params; } diff --git a/shared/fetcher.js b/shared/fetcher.js index 99380c70..2361d207 100644 --- a/shared/fetcher.js +++ b/shared/fetcher.js @@ -227,7 +227,7 @@ Fetcher.prototype.fetchFromApi = function(spec, callback) { body = resp.body; resp.body = typeof body === 'string' ? body.slice(0, 150) : body; respOutput = JSON.stringify(resp); - err = new Error("ERROR fetching model '" + fetcher.modelUtils.modelOrCollectionName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput); + err = new Error("ERROR fetching model '" + fetcher.modelUtils.resourceName(model.constructor) + "' with options '" + JSON.stringify(options) + "'. Response: " + respOutput); err.status = resp.status; err.body = body; callback(err); @@ -257,7 +257,7 @@ Fetcher.prototype.summarize = function(modelOrCollection) { if (this.modelUtils.isCollection(modelOrCollection)) { idAttribute = modelOrCollection.model.prototype.idAttribute; summary = { - collection: this.modelUtils.modelOrCollectionName(modelOrCollection.constructor), + collection: this.modelUtils.resourceName(modelOrCollection.constructor), ids: modelOrCollection.pluck(idAttribute), params: modelOrCollection.params, meta: modelOrCollection.meta @@ -265,7 +265,7 @@ Fetcher.prototype.summarize = function(modelOrCollection) { } else if (this.modelUtils.isModel(modelOrCollection)) { idAttribute = modelOrCollection.idAttribute; summary = { - model: this.modelUtils.modelOrCollectionName(modelOrCollection.constructor), + model: this.modelUtils.resourceName(modelOrCollection.constructor), id: modelOrCollection.get(idAttribute) }; } diff --git a/shared/modelUtils.js b/shared/modelUtils.js index 3544a19c..7e547f62 100644 --- a/shared/modelUtils.js +++ b/shared/modelUtils.js @@ -32,7 +32,7 @@ ModelUtils.prototype.getModel = function(path, attrs, options, callback, fallbac try { Model = this.getModelConstructor(path) } catch (e) { - if (e.code === 'MODULE_NOT_FOUND' && fallbackToBaseModel) { + if ((e.code === 'MODULE_NOT_FOUND' || e.match(/module '.*' not found/)) && fallbackToBaseModel) { Model = BaseModel; } else { throw e; @@ -95,7 +95,7 @@ ModelUtils.prototype.isCollection = function(obj) { ModelUtils.prototype.getModelNameForCollectionName = function(collectionName) { var Collection; Collection = this.getCollectionConstructor(collectionName); - return this.modelOrCollectionName(Collection.prototype.model); + return this.resourceName(Collection.prototype.model); }; ModelUtils.uppercaseRe = /([A-Z])/g; @@ -129,7 +129,7 @@ ModelUtils.prototype.underscorize = function(name) { * -> "" * MyClass.id = "MyClass" */ -ModelUtils.prototype.modelOrCollectionName = function(modelOrCollectionClass) { +ModelUtils.prototype.resourceName = function(modelOrCollectionClass) { return this.underscorize(modelOrCollectionClass.id || modelOrCollectionClass.name); }; diff --git a/shared/store/collection_store.js b/shared/store/collection_store.js index dbfe36c2..4c4914be 100644 --- a/shared/store/collection_store.js +++ b/shared/store/collection_store.js @@ -16,7 +16,7 @@ CollectionStore.prototype.constructor = CollectionStore; CollectionStore.prototype.set = function(collection, params) { var data, idAttribute, key; params = params || collection.params; - key = this._getStoreKey(this.modelUtils.modelOrCollectionName(collection.constructor), params); + key = this._getStoreKey(this.modelUtils.resourceName(collection.constructor), params); idAttribute = collection.model.prototype.idAttribute; data = { ids: collection.pluck(idAttribute), diff --git a/shared/store/model_store.js b/shared/store/model_store.js index 84b00001..ed75cfd5 100644 --- a/shared/store/model_store.js +++ b/shared/store/model_store.js @@ -14,41 +14,41 @@ ModelStore.prototype = Object.create(Super.prototype); ModelStore.prototype.constructor = ModelStore; ModelStore.prototype.set = function(model) { - var existingAttrs, id, key, modelOrCollectionName, newAttrs, constructor; + var existingAttrs, id, key, keyPrefix, newAttrs, constructor; id = model.get(model.idAttribute); - modelOrCollectionName = this.modelUtils.modelOrCollectionName(model.constructor); - if (!modelOrCollectionName && model.collection) { - modelOrCollectionName = this.modelUtils.modelOrCollectionName(model.collection.constructor); + keyPrefix = this.modelUtils.resourceName(model.constructor); + if (!keyPrefix && model.collection) { + keyPrefix = this.modelUtils.resourceName(model.collection.constructor); } /** * If the model is not named and not part of a named collection, * fall back to an empty string to preserve existing behavior. */ - modelOrCollectionName = modelOrCollectionName || ''; - key = this._getModelStoreKey(modelOrCollectionName, id); + keyPrefix = keyPrefix || ''; + key = this._getModelStoreKey(keyPrefix, id); /** * We want to merge the model attrs with whatever is already * present in the store. */ - existingAttrs = this.get(modelOrCollectionName, id) || {}; + existingAttrs = this.get(keyPrefix, id) || {}; newAttrs = _.extend({}, existingAttrs, model.toJSON()); return Super.prototype.set.call(this, key, newAttrs, null); }; -ModelStore.prototype.get = function(modelOrCollectionName, id, returnModelInstance) { +ModelStore.prototype.get = function(resourceName, id, returnModelInstance) { var key, modelData; if (returnModelInstance == null) { returnModelInstance = false; } - key = this._getModelStoreKey(modelOrCollectionName, id); + key = this._getModelStoreKey(resourceName, id); modelData = Super.prototype.get.call(this, key); if (modelData) { if (returnModelInstance) { - return this.modelUtils.getModel(modelOrCollectionName, modelData, { + return this.modelUtils.getModel(resourceName, modelData, { app: this.app }, null, true); } else { @@ -57,9 +57,9 @@ ModelStore.prototype.get = function(modelOrCollectionName, id, returnModelInstan } }; -ModelStore.prototype.find = function(modelOrCollectionName, params) { +ModelStore.prototype.find = function(resourceName, params) { var prefix, foundCachedObject, _this, data, foundCachedObjectKey; - prefix = this._formatKey(this._keyPrefix(modelOrCollectionName)); + prefix = this._formatKey(this._keyPrefix(resourceName)); _this = this; // find the cached object that has attributes which are a subset of the params foundCachedObject = _.find(this.cache, function(cacheObject, key) { @@ -94,10 +94,10 @@ function isObjectSubset(potentialSubset, objectToTest) { }); } -ModelStore.prototype._keyPrefix = function(modelOrCollectionName) { - return this.modelUtils.underscorize(modelOrCollectionName); +ModelStore.prototype._keyPrefix = function(resourceName) { + return this.modelUtils.underscorize(resourceName); } -ModelStore.prototype._getModelStoreKey = function(modelOrCollectionName, id) { - return this._keyPrefix(modelOrCollectionName) + ":" + id; +ModelStore.prototype._getModelStoreKey = function(resourceName, id) { + return this._keyPrefix(resourceName) + ":" + id; } diff --git a/test/shared/store/model_store.test.js b/test/shared/store/model_store.test.js index 78140725..8870746e 100644 --- a/test/shared/store/model_store.test.js +++ b/test/shared/store/model_store.test.js @@ -21,7 +21,7 @@ util.inherits(MyCollection, BaseCollection); function App() {} -addClassMapping.add(modelUtils.modelOrCollectionName(MyModel), MyModel); +addClassMapping.add(modelUtils.resourceName(MyModel), MyModel); describe('ModelStore', function() { beforeEach(function() { @@ -62,7 +62,7 @@ describe('ModelStore', function() { model = new MyCustomModel(modelAttrs); this.store.set(model); - result = this.store.get(modelUtils.modelOrCollectionName(MyCustomModel), modelAttrs.login); + result = this.store.get(modelUtils.resourceName(MyCustomModel), modelAttrs.login); result.should.eql(modelAttrs); }); @@ -106,7 +106,7 @@ describe('ModelStore', function() { } util.inherits(MySecondModel, BaseModel); - addClassMapping.add(modelUtils.modelOrCollectionName(MySecondModel), MySecondModel); + addClassMapping.add(modelUtils.resourceName(MySecondModel), MySecondModel); it('should find a model on custom attributes', function(){ var model, modelAttrs, result; From c0dccfd24675fe95019f587586b2a397b56d4a21 Mon Sep 17 00:00:00 2001 From: shebson Date: Wed, 19 Feb 2014 13:10:18 -0800 Subject: [PATCH 3/3] Merged upstream changes and updated resourceName tests --- test/shared/modelUtils.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/test/shared/modelUtils.test.js b/test/shared/modelUtils.test.js index 2f14881c..fb6165cc 100644 --- a/test/shared/modelUtils.test.js +++ b/test/shared/modelUtils.test.js @@ -280,7 +280,7 @@ describe('modelUtils', function () { }); }); - describe('modelName', function () { + describe('resourceName', function () { var ModelConstructor; beforeEach(function () { @@ -289,11 +289,11 @@ describe('modelUtils', function () { it('should return the underscorized model id if it is set', function () { ModelConstructor.id = 'modelId'; - modelUtils.modelName(ModelConstructor).should.equal('model_id'); + modelUtils.resourceName(ModelConstructor).should.equal('model_id'); }); it('should return the underscorized constructor name if the id is not set', function () { - modelUtils.modelName(ModelConstructor).should.equal('model_name'); + modelUtils.resourceName(ModelConstructor).should.equal('model_name'); }); });