remove passing factories to store methods #3074
Conversation
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
2 times, most recently
from
cff8ce5
to
3aea06b
Compare
|
closes #3072 |
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
3 times, most recently
from
8f443a0
to
7271cf1
Compare
|
I really wish I waited a day before rebasing references, this has been a source of major pain. ❤️ ❤️ Awesome work! |
|
|
||
| var adapter = this.lookupAdapter(type.modelName) || this.lookupAdapter('application'); | ||
| adapterFor: function(modelName) { | ||
| Ember.assert('Passing classes to store methods has been removed. Please pass a dasherized string instead of '+ Ember.inspect(modelName), typeof modelName === 'string'); |
There was a problem hiding this comment.
I think we might want to deprecate here if our own code was doing it.
There was a problem hiding this comment.
I fixed the places our own code was doing it.. Seems odd to ship a version that triggers deprecation warnings?
There was a problem hiding this comment.
I am saying we should fix our own code as well, yes, but if we were doing it, seems like other people could've as well, and it would be nicer not to break their apps.
There was a problem hiding this comment.
Their apps would be broken by everything else asserting anyway.
There was a problem hiding this comment.
The fix is also pretty easy for others to incorporate.
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
4 times, most recently
from
93082ee
to
9c9420f
Compare
| // TODO: Ask Teddy what we should do here. | ||
| // Ideally this should always get passed a string. | ||
|
|
||
| var modelName = typeof modelNameOrFactory === 'string' ? modelNameOrFactory : modelNameOrFactory.modelName; |
There was a problem hiding this comment.
I don't think this gives users actionable feedback as it's in the debug adapter for the Ember Inspector. They probably won't build their own source and fix the deprecation warning. We try to avoid deprecations for code that isn't the user's.
|
SGB |
|
Seems great except for the passing of the store to inverseFor |
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
from
9c9420f
to
2d0af4c
Compare
Previously, we allowed users to either pass factories (subclasses of DS.Model), or a string to store methods. For more consistency in the container-based world where things are looked up through strings, support for looking up via passing a factory has been removed. This is part of a refactor that will remove state (like the store) from Ember Data's factories in order to remove Ember.MODEL_FACTORY_INJECTIONS.
When using multiple stores with Ember Data, we want to avoid storing the state on the factory itself. Because multiple stores currently share the same factory, this means that the first store to look up the factory will store itself as the `store` on that factory. That means that any stores thereafter will receive factories whose `store` property references the first store, not their own.
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
from
3f1a33e
to
05ba8f8
Compare
| throw new Ember.Error("No model was found for '" + modelName + "'"); | ||
| } | ||
| factory.modelName = factory.modelName || normalizeModelName(modelName); | ||
| factory.__container__ = factory.__container__ || container; |
There was a problem hiding this comment.
I am very suspicious of this. Seems like it would break for multiple app runs server side. cc @stefanpenner
There was a problem hiding this comment.
Static methods just can't return concrete references to other factories. This doesn't appear to be used internally at all:
The following could be made to work: typeForRelationship(typeString, store) but not the single arg form.
The question is, as the rest of the system understands strings, why does this need to return a concrete reference in the first place.
There was a problem hiding this comment.
Although this may appear drastic, this appears like the last lynch-pin preventing ED from being a good citizen in this area.
There was a problem hiding this comment.
We need a factory in order to be able to reflect on it. To say if I am a user and I have message, does that message have any relationships that point to a user
There was a problem hiding this comment.
Can you show me that code? Or provide an expanded scenario by which i can continue to think about this?
There was a problem hiding this comment.
in slack we discussed this further. I provided the following example:
As nothing prevents two apps from having two different set of resolvers, factory level reflection without polluting instance state isn't really available to us.
The solution is to continue to purge instance state (the container/ store etc) from factories, and instead enable the needing wiring when instance information is present.
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
from
05ba8f8
to
bcefa52
Compare
…factories-to-store-methods
…factories-to-store-methods
…factories-to-store-methods
fivetanley
force-pushed
the
remove-passing-factories-to-store-methods
branch
from
4f621da
to
c3197d6
Compare
…factories-to-store-methods
…re-methods remove passing factories to store methods
|
🎉 |
|
👯 |
Previously, we allowed users to either pass factories (subclasses of
DS.Model), or a string to store methods. For more consistency in the
container-based world where things are looked up through strings,
support for looking up via passing a factory has been removed.
This is part of a refactor that will remove state (like the store)
from Ember Data's factories in order to remove
Ember.MODEL_FACTORY_INJECTIONS.