From ee9776056922ff33661cb60f775519c4902c4657 Mon Sep 17 00:00:00 2001 From: Stanley Stuart Date: Fri, 29 May 2015 11:20:32 -0500 Subject: [PATCH] refactor store managed instances In this commit, we refactor store managed instance lookups to have its own class that takes the store's container. This moves a large portion of code out of the store and instead uses delegation. There's also a performance benefit: If you look up `adapter:post` and fallback to `adapter:application`, this lookup is cached. So, if you look up `adapter:post` again, that instance will actually be cached. Previously, we did the lookup for `adapter:post` every time. This also removes passing factories as the `adapter` property to DS.Store. closes #3050 --- packages/ember-data/lib/system/adapter.js | 1 + packages/ember-data/lib/system/store.js | 69 ++++----- .../system/store/container-instance-cache.js | 86 +++++++++++ .../tests/helpers/custom-adapter.js | 10 ++ .../tests/integration/filter-test.js | 50 +++---- .../tests/unit/store/adapter-interop-test.js | 7 +- .../tests/unit/store/lookup-test.js | 135 ++++++++++++++++++ tests/ember-configuration.js | 10 +- 8 files changed, 294 insertions(+), 74 deletions(-) create mode 100644 packages/ember-data/lib/system/store/container-instance-cache.js create mode 100644 packages/ember-data/tests/helpers/custom-adapter.js create mode 100644 packages/ember-data/tests/unit/store/lookup-test.js diff --git a/packages/ember-data/lib/system/adapter.js b/packages/ember-data/lib/system/adapter.js index cc9295ca0f9..68274a76d79 100644 --- a/packages/ember-data/lib/system/adapter.js +++ b/packages/ember-data/lib/system/adapter.js @@ -83,6 +83,7 @@ var Adapter = Ember.Object.extend({ @property defaultSerializer @type {String} */ + defaultSerializer: '-default', /** The `find()` method is invoked when the store is asked for a record that diff --git a/packages/ember-data/lib/system/store.js b/packages/ember-data/lib/system/store.js index 2f385b029c9..53c19974ae1 100644 --- a/packages/ember-data/lib/system/store.js +++ b/packages/ember-data/lib/system/store.js @@ -7,8 +7,7 @@ import normalizeModelName from "ember-data/system/normalize-model-name"; import { - InvalidError, - Adapter + InvalidError } from "ember-data/system/adapter"; import { Map @@ -41,6 +40,7 @@ import { import coerceId from "ember-data/system/coerce-id"; import RecordArrayManager from "ember-data/system/record-array-manager"; +import ContainerInstanceCache from 'ember-data/system/store/container-instance-cache'; import InternalModel from "ember-data/system/model/internal-model"; import Model from "ember-data/system/model"; @@ -214,7 +214,7 @@ Store = Service.extend({ store: this }); this._pendingSave = []; - this._containerCache = Ember.create(null); + this._instanceCache = new ContainerInstanceCache(this.container); //Used to keep track of all the find requests that need to be coalesced this._pendingFetch = Map.create(); }, @@ -273,18 +273,9 @@ Store = Service.extend({ defaultAdapter: Ember.computed('adapter', function() { var adapter = get(this, 'adapter'); - Ember.assert('You tried to set `adapter` property to an instance of `DS.Adapter`, where it should be a name or a factory', !(adapter instanceof Adapter)); + Ember.assert('You tried to set `adapter` property to an instance of `DS.Adapter`, where it should be a name', typeof adapter === 'string'); - if (typeof adapter === 'string') { - adapter = this.container.lookup('adapter:' + adapter) || this.container.lookup('adapter:application') || this.container.lookup('adapter:-rest'); - } - - if (DS.Adapter.detect(adapter)) { - adapter = adapter.create({ - container: this.container, - store: this - }); - } + adapter = this.retrieveManagedInstance('adapter', adapter); return adapter; }), @@ -1888,9 +1879,7 @@ Store = Service.extend({ modelName = modelOrClass; } - var adapter = this.lookupAdapter(modelName) || this.lookupAdapter('application'); - - return adapter || get(this, 'defaultAdapter'); + return this.lookupAdapter(modelName); }, _adapterRun: function (fn) { @@ -1932,17 +1921,13 @@ Store = Service.extend({ modelName = modelOrClass; } - var serializer = this.lookupSerializer(modelName) || this.lookupSerializer('application'); - - if (!serializer) { - var adapter = this.adapterFor(modelName); - serializer = this.lookupSerializer(get(adapter, 'defaultSerializer')); - } - - if (!serializer) { - serializer = this.lookupSerializer('-default'); - } + var fallbacks = [ + 'application', + this.adapterFor(modelName).get('defaultSerializer'), + '-default' + ]; + var serializer = this.lookupSerializer(modelName, fallbacks); return serializer; }, @@ -1958,30 +1943,28 @@ Store = Service.extend({ @private @param {String} modelName the object modelName @param {String} name the object name + @param {Array} fallbacks the fallback objects to lookup if the lookup for modelName or 'application' fails @return {Ember.Object} */ - retrieveManagedInstance: function(modelName, name) { - var normalizedTypeKey = normalizeModelName(modelName); - var key = normalizedTypeKey + ":" +name; - - if (!this._containerCache[key]) { - var instance = this.container.lookup(key); - - if (instance) { - set(instance, 'store', this); - this._containerCache[key] = instance; - } - } + retrieveManagedInstance: function(type, modelName, fallbacks) { + var normalizedModelName = normalizeModelName(modelName); - return this._containerCache[key]; + var instance = this._instanceCache.get(type, normalizedModelName, fallbacks); + set(instance, 'store', this); + return instance; }, lookupAdapter: function(name) { - return this.retrieveManagedInstance('adapter', name); + return this.retrieveManagedInstance('adapter', name, this.get('_adapterFallbacks')); }, - lookupSerializer: function(name) { - return this.retrieveManagedInstance('serializer', name); + _adapterFallbacks: Ember.computed('adapter', function() { + var adapter = this.get('adapter'); + return ['application', adapter, '-rest']; + }), + + lookupSerializer: function(name, fallbacks) { + return this.retrieveManagedInstance('serializer', name, fallbacks); }, willDestroy: function() { diff --git a/packages/ember-data/lib/system/store/container-instance-cache.js b/packages/ember-data/lib/system/store/container-instance-cache.js new file mode 100644 index 00000000000..cc86980874d --- /dev/null +++ b/packages/ember-data/lib/system/store/container-instance-cache.js @@ -0,0 +1,86 @@ +import Ember from 'ember'; + +/** + * The `ContainerInstanceCache` serves as a lazy cache for looking up + * instances of serializers and adapters. It has some additional logic for + * finding the 'fallback' adapter or serializer. + * + * The 'fallback' adapter or serializer is an adapter or serializer that is looked up + * when the preferred lookup fails. For example, say you try to look up `adapter:post`, + * but there is no entry (app/adapters/post.js in EmberCLI) for `adapter:post` in the registry. + * + * The `fallbacks` array passed will then be used; the first entry in the fallbacks array + * that exists in the container will then be cached for `adapter:post`. So, the next time you + * look up `adapter:post`, you'll get the `adapter:application` instance (or whatever the fallback + * was if `adapter:application` doesn't exist). + * + * @private + * @class ContainerInstanceCache + * +*/ +function ContainerInstanceCache(container) { + this._container = container; + this._cache = Ember.create(null); +} + +ContainerInstanceCache.prototype = Ember.create(null); + +Ember.merge(ContainerInstanceCache.prototype, { + get: function(type, preferredKey, fallbacks) { + let cache = this._cache; + let preferredLookupKey = `${type}:${preferredKey}`; + + if (!(preferredLookupKey in cache)) { + let instance = this.instanceFor(preferredLookupKey) || this._findInstance(type, fallbacks); + if (instance) { + cache[preferredLookupKey] = instance; + } + } + return cache[preferredLookupKey]; + }, + + _findInstance: function(type, fallbacks) { + for (let i = 0, length = fallbacks.length; i < length; i++) { + let fallback = fallbacks[i]; + let lookupKey = `${type}:${fallback}`; + let instance = this.instanceFor(lookupKey); + + if (instance) { + return instance; + } + } + }, + + instanceFor: function(key) { + let cache = this._cache; + if (!cache[key]) { + let instance = this._container.lookup(key); + if (instance) { + cache[key] = instance; + } + } + return cache[key]; + }, + + destroy: function() { + let cache = this._cache; + let cacheEntries = Ember.keys(cache); + + for (let i = 0, length = cacheEntries.length; i < length; i++) { + let cacheKey = cacheEntries[i]; + let cacheEntry = cache[cacheKey]; + if (cacheEntry) { + cacheEntry.destroy(); + } + } + this._container = null; + }, + + constructor: ContainerInstanceCache, + + toString: function() { + return 'ContainerInstanceCache'; + } +}); + +export default ContainerInstanceCache; diff --git a/packages/ember-data/tests/helpers/custom-adapter.js b/packages/ember-data/tests/helpers/custom-adapter.js new file mode 100644 index 00000000000..3cead14285f --- /dev/null +++ b/packages/ember-data/tests/helpers/custom-adapter.js @@ -0,0 +1,10 @@ + +export default function(env, adapterDefinition) { + var adapter = adapterDefinition; + if (!DS.Adapter.detect(adapterDefinition)) { + adapter = DS.Adapter.extend(adapterDefinition); + } + var store = env.store; + env.registry.register('adapter:-custom', adapter); + Ember.run(() => store.set('adapter', '-custom')); +} diff --git a/packages/ember-data/tests/integration/filter-test.js b/packages/ember-data/tests/integration/filter-test.js index e59721e90ee..9d6503a4f6b 100644 --- a/packages/ember-data/tests/integration/filter-test.js +++ b/packages/ember-data/tests/integration/filter-test.js @@ -1,3 +1,5 @@ +import customAdapter from 'ember-data/tests/helpers/custom-adapter'; + var get = Ember.get; var set = Ember.set; var forEach = Ember.EnumerableUtils.forEach; @@ -168,13 +170,11 @@ test("a filtered record array includes created elements", function() { }); test("a Record Array can update its filter", function() { - run(function() { - set(store, 'adapter', DS.Adapter.extend({ - deleteRecord: function(store, type, snapshot) { - return Ember.RSVP.resolve(); - } - })); - }); + customAdapter(env, DS.Adapter.extend({ + deleteRecord: function(store, type, snapshot) { + return Ember.RSVP.resolve(); + } + })); run(function() { store.pushMany('person', array); @@ -227,13 +227,11 @@ test("a Record Array can update its filter", function() { }); test("a Record Array can update its filter and notify array observers", function() { - run(function() { - set(store, 'adapter', DS.Adapter.extend({ - deleteRecord: function(store, type, snapshot) { - return Ember.RSVP.resolve(); - } - })); - }); + customAdapter(env, DS.Adapter.extend({ + deleteRecord: function(store, type, snapshot) { + return Ember.RSVP.resolve(); + } + })); run(function() { store.pushMany('person', array); @@ -367,7 +365,7 @@ test("a filter created after a record is already loaded works", function() { }); test("filter with query persists query on the resulting filteredRecordArray", function() { - set(store, 'adapter', DS.Adapter.extend({ + customAdapter(env, DS.Adapter.extend({ findQuery: function(store, type, id) { return Ember.RSVP.resolve([{ id: id, @@ -375,6 +373,7 @@ test("filter with query persists query on the resulting filteredRecordArray", fu }]); } })); + var filter; run(function() { @@ -393,13 +392,14 @@ test("filter with query persists query on the resulting filteredRecordArray", fu test("it is possible to filter by state flags", function() { var filter; - run(function() { - set(store, 'adapter', DS.Adapter.extend({ - find: function(store, type, id, snapshot) { - return Ember.RSVP.resolve({ id: id, name: "Tom Dale" }); - } - })); + customAdapter(env, DS.Adapter.extend({ + find: function(store, type, id, snapshot) { + return Ember.RSVP.resolve({ id: id, name: "Tom Dale" }); + } + })); + + run(function() { filter = store.filter('person', function(person) { return person.get('isLoaded'); }); @@ -423,7 +423,7 @@ test("it is possible to filter by state flags", function() { }); test("it is possible to filter loaded records by dirtiness", function() { - set(store, 'adapter', DS.Adapter.extend({ + customAdapter(env, DS.Adapter.extend({ updateRecord: function() { return Ember.RSVP.resolve(); } @@ -456,7 +456,7 @@ test("it is possible to filter loaded records by dirtiness", function() { test("it is possible to filter created records by dirtiness", function() { run(function() { - set(store, 'adapter', DS.Adapter.extend({ + customAdapter(env, DS.Adapter.extend({ createRecord: function() { return Ember.RSVP.resolve(); } @@ -490,7 +490,7 @@ test("it is possible to filter created records by dirtiness", function() { }); test("it is possible to filter created records by isReloading", function() { - set(store, 'adapter', DS.Adapter.extend({ + customAdapter(env, DS.Adapter.extend({ find: function(store, type, id, snapshot) { return Ember.RSVP.resolve({ id: 1, @@ -550,7 +550,7 @@ var serverResponds = function() { var setup = function(serverCallbacks) { run(function() { - set(store, 'adapter', DS.Adapter.extend(serverCallbacks)); + customAdapter(env, DS.Adapter.extend(serverCallbacks)); store.pushMany('person', array); diff --git a/packages/ember-data/tests/unit/store/adapter-interop-test.js b/packages/ember-data/tests/unit/store/adapter-interop-test.js index 7bf95b7164e..1b1a803c6b7 100644 --- a/packages/ember-data/tests/unit/store/adapter-interop-test.js +++ b/packages/ember-data/tests/unit/store/adapter-interop-test.js @@ -28,15 +28,12 @@ test('Adapter can be set as a name', function() { }); test('Adapter can not be set as an instance', function() { - expect(5); + expect(1); store = DS.Store.create({ adapter: DS.Adapter.create() }); - var assert = Ember.assert; - Ember.assert = function() { ok(true, "raises an error when passing in an instance"); }; - store.get('defaultAdapter'); - Ember.assert = assert; + expectAssertion(() => store.get('defaultAdapter')); }); test("Calling Store#find invokes its adapter#find", function() { diff --git a/packages/ember-data/tests/unit/store/lookup-test.js b/packages/ember-data/tests/unit/store/lookup-test.js new file mode 100644 index 00000000000..6e942aefdc7 --- /dev/null +++ b/packages/ember-data/tests/unit/store/lookup-test.js @@ -0,0 +1,135 @@ +var store, env, applicationAdapter, applicationSerializer, Person; +const run = Ember.run; + +function resetStore() { + if (store) { + run(store, 'destroy'); + } + env = setupStore({ + adapter: '-rest', + person: Person + }); + + env.registry.unregister('adapter:application'); + env.registry.unregister('serializer:application'); + + env.registry.optionsForType('serializer', { singleton: true }); + env.registry.optionsForType('adapter', { singleton: true }); + + store = env.store; +} + +function lookupAdapter(adapterName) { + return run(store, 'adapterFor', adapterName); +} + +function lookupSerializer(serializerName) { + return run(store, 'serializerFor', serializerName); +} + +function registerAdapter(adapterName, adapter) { + env.registry.register(`adapter:${adapterName}`, adapter); +} + +function registerSerializer(serializerName, serializer) { + env.registry.register(`serializer:${serializerName}`, serializer); +} + +module('unit/store/lookup - Managed Instance lookups', { + setup() { + Person = DS.Model.extend(); + resetStore(); + env.registry.register('adapter:application', DS.Adapter.extend()); + env.registry.register('adapter:serializer', DS.Adapter.extend()); + + applicationAdapter = run(store, 'adapterFor', 'application'); + applicationSerializer = run(store, 'serializerFor', 'application'); + }, + + teardown() { + run(store, 'destroy'); + } +}); + +test('when the adapter does not exist for a type, the fallback is returned', () => { + let personAdapter = lookupAdapter('person'); + + strictEqual(personAdapter, applicationAdapter); +}); + +test('when the adapter for a type exists, returns that instead of the fallback', () => { + registerAdapter('person', DS.Adapter.extend()); + let personAdapter = lookupAdapter('person'); + + ok(personAdapter !== applicationAdapter); +}); + +test('when the serializer does not exist for a type, the fallback is returned', () => { + let personSerializer = lookupSerializer('person'); + + strictEqual(personSerializer, applicationSerializer); +}); + +test('when the serializer does exist for a type, the serializer is returned', () => { + registerSerializer('person', DS.Serializer.extend()); + + let personSerializer = lookupSerializer('person'); + + ok(personSerializer !== applicationSerializer); +}); + +test('adapter lookup order', () => { + expect(3); + + resetStore(); + + let personAdapter = lookupAdapter('person'); + + strictEqual(personAdapter, lookupAdapter('-rest'), 'looks up the RESTAdapter first'); + resetStore(); + + registerAdapter('application', DS.ActiveModelSerializer.extend()); + personAdapter = lookupAdapter('person'); + + strictEqual(personAdapter, lookupAdapter('application'), 'looks up application adapter before RESTAdapter if it exists'); + + resetStore(); + + registerAdapter('application', DS.ActiveModelSerializer.extend()); + registerAdapter('person', DS.ActiveModelSerializer.extend({ customThingy: true })); + + ok(lookupAdapter('person').get('customThingy'), 'looks up type serializer before application'); +}); + +test('serializer lookup order', () => { + resetStore(); + + let personSerializer = lookupSerializer('person'); + + strictEqual(personSerializer, lookupSerializer('-rest')); + + resetStore(); + + registerSerializer('application', DS.ActiveModelSerializer.extend()); + personSerializer = lookupSerializer('person'); + strictEqual(personSerializer, lookupSerializer('application'), 'looks up application before default'); + + resetStore(); + registerAdapter('person', DS.Adapter.extend({ + defaultSerializer: '-active-model' + })); + personSerializer = lookupSerializer('person'); + + strictEqual(personSerializer, lookupSerializer('-active-model'), 'uses defaultSerializer on adapterFor("model") if application not defined'); + + resetStore(); + registerAdapter('person', DS.Adapter.extend({ + defaultSerializer: '-active-model' + })); + registerSerializer('application', DS.ActiveModelSerializer.extend()); + registerSerializer('person', DS.JSONSerializer.extend({ customThingy: true })); + personSerializer = lookupSerializer('person'); + + ok(personSerializer.get('customThingy'), 'uses the person serializer before any fallbacks if it is defined'); +}); + diff --git a/tests/ember-configuration.js b/tests/ember-configuration.js index b0582a9eb85..1a71c4afc9a 100644 --- a/tests/ember-configuration.js +++ b/tests/ember-configuration.js @@ -67,9 +67,14 @@ } }; - var adapter = env.adapter = (options.adapter || DS.Adapter); + var adapter = env.adapter = (options.adapter || '-default'); delete options.adapter; + if (typeof adapter !== 'string') { + env.registry.register('adapter:-ember-data-test-custom', adapter); + adapter = '-ember-data-test-custom'; + } + for (var prop in options) { registry.register('model:' + Ember.String.dasherize(prop), options[prop]); } @@ -80,9 +85,12 @@ registry.optionsForType('serializer', { singleton: false }); registry.optionsForType('adapter', { singleton: false }); + registry.register('adapter:-default', DS.Adapter); registry.register('serializer:-default', DS.JSONSerializer); registry.register('serializer:-rest', DS.RESTSerializer); + registry.register('adapter:-active-model', DS.ActiveModelAdapter); + registry.register('serializer:-active-model', DS.ActiveModelSerializer); registry.register('adapter:-rest', DS.RESTAdapter); registry.injection('serializer', 'store', 'store:main');