Store Managed Adapters & Serializers #2617

jmurphyau · 2014-12-19T14:17:41Z

Pull request to make a store instance responsible for it's own adapter and serializer instances - as suggested fix to #2588 by @tomdale.

Serializers and Adapters are no longer singletons.

Failing test included.

igorT · 2014-12-19T14:50:41Z

packages/ember-data/lib/system/store.js

@@ -185,6 +185,8 @@ Store = Ember.Object.extend({
      store: this
    });
    this._pendingSave = [];
+    this._adapterInstances = {};


Object.create(null) seems appropriate for this

igorT · 2014-12-19T14:56:01Z

Thanks for the PR, left some comments

jmurphyau · 2014-12-19T17:29:34Z

Thanks for the comments @igorT. Didn't know about Object.create(null) before now (looked it up).

The addition of lookupSerializer and lookupAdapter in the commit was due to the complexity of the 4 different lookups performed in serializerFor (typeKey, 'application', adapter.defaultSerializer, '-default') overcrowding the function.

Having to:

Check if the serializer is in the cache
If not found, then look up the serializer in the container
If successfully looked up/created, add the store as a parameter and add to cache

For potentially 4 different serializer types is the justification - and then if you have lookupSerializer why not be consistent with lookupAdapter?

I wasn't sure if it should be a function outside of the object (such as serializerFor, defaultSerializer, serializerForAdapter, at the bottom of the file) or a function on the instance.

sly7-7 · 2014-12-19T19:30:06Z

packages/ember-data/lib/system/store.js

@@ -250,7 +252,8 @@ Store = Ember.Object.extend({

    if (DS.Adapter.detect(adapter)) {


@igorT @stefanpenner Do we still want passing a class here ?

I think so because we just looked it up couple lines above?

This seems inconsistent to me. I mean, I thought we now should always lookup adapter/serializer via the container and not instanciating it. Also, we encourage strings everywhere, but in this case we still want to fully support a store definition with an adapter property a class ?

Isn't the adapter here a result of the looked up string?

hum, ok, I'm probably missing something, I thought the lookup returns an instance, not a class.

Ah maybe you're right i thought you meant class vs a string

Well, I think I'm just too bad explaining what I have in mind 😖
So I will try again.

Historically, the user can affect a class or a string to the adapter property. As a result, there is this chain of checks. If the adapter property is a string, then the adapter is looked up via the container, or if it's a class (actually a subclass of DS.Adapter), then it is instantiated here.

My question is: do we still want to allow the user define the adapter property as a class ? It seems to me this would be inconsistent with the "string everywhere" paradigm.

Please, tell me it's clear now... or tell me to take an intensive English training course 😸

jmurphyau · 2014-12-20T00:33:32Z

@sly7-7 (and others) in regards to removing adapter: '-rest'

I think if the order of lookup is/will change it might be an opportunity to simplify both the adapter and serializer lookup preferences..

Current preference/order

Adapters:

1 adapterFor: typeKey
2 adapterFor: 'application'
3 defaultAdapter: this.adapter (default to -rest)
4 defaultAdapter: 'application'
5 defaultAdapter: '-rest'

Serializers:

1 serializerFor: typeKey
2 serializerFor: 'application'
3 serializerFor: adapterFor(typeKey).defaultSerializer (this could return either the type specific adapter, the application wide adapter or the -rest adapter)
4 serializerFor: '-default'

Simplified

Adapters:

Remove this.adapter = '-rest' from the store making it only in use if the user provides it
Remove adapter:application lookups in adapterFor

Which would result in:
1 adapterFor: typeKey
2 defaultAdapter: this.adapter (default to undefined)
3 defaultAdapter: 'application'
4 defaultAdapter: '-rest'

Serializers:

Add defaultSerializer with the same logic as defaultAdapter, returning (in order): serializer:[this.serializer], serializer:application, serializer:-default
Remove references to serializer:application and serializer:-default from serializerFor and instead return the defaultSerializer
In serializerFor, be more explicit in using the defaultSerializer property from an adapter - use lookupAdapter(typeKey) which will only return the adapter for that type (not a default adapter such as 'application' or '-rest') and use the defaultSerializer property on that adapter.

Which would result in:
1 serializerFor: typeKey
2 serializerFor: this.lookupAdapter(typeKey).defaultSerializer
3 defaultSerializer: this.serializer (default to undefined)
4 defaultSerializer: 'application'
5 defaultSerializer: '-default'

igorT · 2015-01-06T05:26:56Z

@jmurphyau apologies for not getting back to you sooner. Getting up to speed now after the holidays. Wouldn't the simplified order prevent people from having custom adapters for custom stores?

igorT · 2015-01-06T05:27:06Z

Can you also please rebase/squash the PR?

igorT · 2015-01-06T05:28:18Z

Awesome work btw @jmurphyau

jmurphyau · 2015-01-07T09:42:04Z

@igorT For a custom adapter on a custom store, this currently exists as item 3 in the current implementation:

defaultSerializer: this.adapter (default to -rest)

and item 2 in what I've documented as a simplified preference/order:

defaultAdapter: this.adapter (default to undefined)

Also, something worth noting that may not be completely obvious to everyone reading this PR - the "simplified preference/order" I've detailed in the comments above is not actually implemented in this PR - this PR is only about allowing multiple stores to work with the embedded record mixin.

Should I split out the preference/order discussion into something/somewhere else? If so, where is appropriate?

jmurphyau · 2015-01-07T10:04:18Z

I think I've rebased this incorrectly - I'm not 100% sure??

Sorry guys don't really have much experience with Git.. I'll investigate see if I've fucked it up/need to change it..

igorT · 2015-01-07T10:14:06Z

Can you try
git reset --hard 3e424a46a563083600570bf00f72759247c8bf5c when you are in your branch
then force push the branch?

jmurphyau · 2015-01-07T10:19:30Z

Done - that looks much better. Thanks @igorT.

I think using GitHub for Mac may have caused part of the problem

bmac · 2015-01-09T23:01:16Z

packages/ember-data/tests/integration/setup-container-test.js

@@ -38,7 +38,7 @@ test("the deprecated serializer:_default is resolved as serializer:default", fun
    deprecated = container.lookup('serializer:_default');
  });

-  ok(deprecated === valid, "they should resolve to the same thing");
+  ok(deprecated.constructor === valid.constructor, "they should resolve to the same thing");


Can you add a test that assert your config for singleton:false

bmac · 2015-02-12T22:10:04Z

ping @jmurphyau

jmurphyau · 2015-02-14T09:30:09Z

@bmac - I must have missed the last few comments from you.

I'll split out those tests & add the tests you mentioned over the next day or so.

jmurphyau · 2015-02-14T11:23:19Z

@bmac - I've split the tests out - please review.

jmurphyau · 2015-02-25T13:01:56Z

@bmac @igorT @sly7-7 - any further thoughts/suggestions on this PR?

stefanpenner · 2015-02-25T14:21:00Z

packages/ember-data/lib/system/store.js

@@ -185,6 +185,7 @@ Store = Ember.Object.extend({
      store: this
    });
    this._pendingSave = [];
+    this._containerCache = Ember.create(null);


why do we need a cache?

It's where the store holds its Serializers and Adapters instances - since each is now one instance per store rather than a singleton.

on destroy, we should be sure to also destroy these. So they can do needed cleanup

Ok cool - I'll get onto that

Adapters and Serializers are now store managed - with each store instance holding a unique instance of each serializer and adapter. They are no longer singletons. This allows multiple stores to be used with ember-data.

jmurphyau · 2015-02-26T08:12:07Z

@stefanpenner - I've made the changes discussed and a few additional ones since the serializerForAdapter function has moved.

These calls are not required here.

Since multiple store tests are now their own file and the secondary stores are created in setup() there’s no need to create secondary stores in this test

igorT · 2015-03-22T21:15:07Z

Thanks for the awesome work! We rebased it and merged it, but kept you as an author. Thanks!

jmurphyau · 2015-03-24T09:55:11Z

Thanks everyone for all you help & feedback with this PR - it was my first PR ever. Was a good experience and I've since learnt a lot about Git and GitHub and moved away from GitHub for Mac in favour of the command line - so shouldn't have any issues with rebase again :)

I've created two other PRs that have stemmed from this PR

#2918 Remove serializerForAdapter

This has stemmed from @bmac's issue #2916 in relation to keeping things consistent.

#2919 Serializer & Adapter Lookups Preference/Order

This has stemmed from the above discussion started by @sly7-7 around the lookup order of serializers and adapters. Parts of the discussion are hidden behind outdated diffs (e.g. it started in 'sly7-7 commented on an outdated diff on Dec 20, 2014')

Serializer and adapter order/preference was discussed above but nothing really appeared to be decided - so I've started with a base - something to go off - which I hope gets things moving in this regard.
It can be changed or improved on or just closed (if now is not the time for this type of change, or if it's not a good idea)

igorT reviewed Dec 19, 2014
View reviewed changes

sly7-7 reviewed Dec 19, 2014
View reviewed changes

sly7-7 mentioned this pull request Dec 20, 2014

fix emberjs/data/issues/2032 - add modelFor to adapterFor #2044

Closed

This was referenced Dec 31, 2014

Ember Data Meeting PR Discussion 2014/12/17 #2606

Closed

Ember Data Meeting PR Discussion 2014/12/31 #2646

Closed

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch from 45d9c6e to 3e424a4 Compare January 7, 2015 10:17

bmac reviewed Jan 9, 2015
View reviewed changes

bmac added this to the 1.0.0-beta.15 milestone Feb 10, 2015

bmac removed this from the 1.0.0-beta.15 milestone Feb 13, 2015

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch 2 times, most recently from b362922 to ed0690c Compare February 14, 2015 12:13

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch from ed0690c to 943d891 Compare February 14, 2015 12:24

stefanpenner reviewed Feb 25, 2015
View reviewed changes

Adapters and Serializers are Store Managed

6ac7380

Adapters and Serializers are now store managed - with each store instance holding a unique instance of each serializer and adapter. They are no longer singletons. This allows multiple stores to be used with ember-data.

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch from 943d891 to 09f4fae Compare February 26, 2015 07:29

Clear the _containerCache on store destroy

9fcf261

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch from 09f4fae to 9fcf261 Compare February 26, 2015 07:32

Changed serializer lookup functions to use store serializer instances

781c48b

jmurphyau force-pushed the 2588-store-managed-adapters-serializers branch from 4587ce1 to 781c48b Compare February 27, 2015 03:20

jmurphyau added 2 commits February 27, 2015 14:26

Removed calls to modelFor

7b10754

These calls are not required here.

Removed unnecessary code

7c9849b

Since multiple store tests are now their own file and the secondary stores are created in setup() there’s no need to create secondary stores in this test

bmac added this to the 1.0.0-beta.16 milestone Feb 28, 2015

bmac mentioned this pull request Mar 22, 2015

Adapters and Serializers are Store Managed #2915

Merged

igorT closed this Mar 22, 2015

bmac mentioned this pull request Mar 22, 2015

should serializerForAdapter take the store as an argument? #2916

Closed

jmurphyau mentioned this pull request Mar 23, 2015

Serializer & Adapter Lookups Preference/Order #2919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Store Managed Adapters & Serializers #2617

Store Managed Adapters & Serializers #2617

jmurphyau commented Dec 19, 2014

igorT Dec 19, 2014

igorT commented Dec 19, 2014

jmurphyau commented Dec 19, 2014

sly7-7 Dec 19, 2014

igorT Jan 6, 2015

sly7-7 Jan 7, 2015

bmac Jan 9, 2015

sly7-7 Jan 10, 2015

igorT Jan 10, 2015

sly7-7 Jan 10, 2015

jmurphyau commented Dec 20, 2014

igorT commented Jan 6, 2015

igorT commented Jan 6, 2015

igorT commented Jan 6, 2015

jmurphyau commented Jan 7, 2015

jmurphyau commented Jan 7, 2015

igorT commented Jan 7, 2015

jmurphyau commented Jan 7, 2015

bmac Jan 9, 2015

bmac commented Feb 12, 2015

jmurphyau commented Feb 14, 2015

jmurphyau commented Feb 14, 2015

jmurphyau commented Feb 25, 2015

stefanpenner Feb 25, 2015

jmurphyau Feb 25, 2015

stefanpenner Feb 25, 2015

stefanpenner Feb 25, 2015

jmurphyau Feb 26, 2015

stefanpenner Feb 26, 2015

jmurphyau commented Feb 26, 2015

igorT commented Mar 22, 2015

jmurphyau commented Mar 24, 2015

		@@ -250,7 +252,8 @@ Store = Ember.Object.extend({

		if (DS.Adapter.detect(adapter)) {

Store Managed Adapters & Serializers #2617

Store Managed Adapters & Serializers #2617

Conversation

jmurphyau commented Dec 19, 2014

Choose a reason for hiding this comment

igorT commented Dec 19, 2014

jmurphyau commented Dec 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmurphyau commented Dec 20, 2014

Current preference/order

Adapters:

Serializers:

Simplified

Adapters:

Serializers:

igorT commented Jan 6, 2015

igorT commented Jan 6, 2015

igorT commented Jan 6, 2015

jmurphyau commented Jan 7, 2015

jmurphyau commented Jan 7, 2015

igorT commented Jan 7, 2015

jmurphyau commented Jan 7, 2015

Choose a reason for hiding this comment

bmac commented Feb 12, 2015

jmurphyau commented Feb 14, 2015

jmurphyau commented Feb 14, 2015

jmurphyau commented Feb 25, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmurphyau commented Feb 26, 2015

igorT commented Mar 22, 2015

jmurphyau commented Mar 24, 2015