[BUGFIX DEPRECATE] fixes and deprecates when a user re-exports a model multiple places for use #4886
Conversation
runspired
changed the title
There was a problem hiding this comment.
Being able to re-export a class is a feature of ember data today. As we discussed oob, the factoryFor changes have broken this in subtle ways as you can get the wrong modelName on a re-exported model, which can get used to build URLs in adapters &c.
But we should be able to deal with this more gracefully for Ember Data users by deprecating with a double extend when we detect this has happened.
So, when we detect that a class already has a modelName we double extend and warn with a deprecation.
thoughts?
|
I like the suggestion from @hjdivad. Since @stefanpenner is out, perhaps @krisselden or @rwjblue has thoughts on the best approach here? |
runspired
changed the title
|
@hjdivad updated |
runspired
changed the title
runspired
changed the title
addon/-private/system/store.js
Outdated
| `import Model from './${klass.modelName}';\n\n\texport default Model.extend();`; | ||
|
|
||
| if (hasFactoryFor) { | ||
| klass = factory.class = klass.extend(); |
There was a problem hiding this comment.
It seems like a bug to me that ember allows setting factory.class like this...
There was a problem hiding this comment.
if it is, then I don't think we have a path forward other than to throw an error. Would be only slightly breaking.
There was a problem hiding this comment.
factoryFor isn’t supposed to allow that, I thought we were using proxies to error in debug for that.
There was a problem hiding this comment.
@krisselden - We do exactly that, for all other properties other than create and class. We should fix this (e.g. throw if class or create are attempted to be set).
There was a problem hiding this comment.
@runspired - We could still do what you want I think. Either:
- do the double extend when we detect modelName is not a match, register, then look up again for them...
- create an cache our own "fake factory" here...
There was a problem hiding this comment.
I think I prefer the register approach given it is "more correct" in terms of intent. Would be weird if ED and the registry were out of sync.
There was a problem hiding this comment.
@rwjblue / @krisselden i totally agree with you guys, the issue is ember-data's reliance on constructor.modelName. I have removed as much of this dependence as can (I believe) without public API change...
There was a problem hiding this comment.
I believe if users have tests that pollute the constructor, they should subclass in the own tests. I don't believe we should subclass for them.
addon/-private/system/store.js
Outdated
| to continue allowing this ignores the case where modelName is set but is | ||
| the same. | ||
| */ | ||
| if (klass.modelName && klass.modelName !== modelName) { |
There was a problem hiding this comment.
In theory (for the test case), we could detect if the original container has been destroyed or changed and if so, just seamlessly transition without a warning?
There was a problem hiding this comment.
ugh, I hadn't even considered tests mutating this yet. :'(
Thoughts on how would we differentiate between "modelName set by previous container" and "modelName set by previous location's lookup"?
There was a problem hiding this comment.
if the owner is different? Maybe we store an owner version.
- I think tests should just double extend themselves (this is aligned with how the language works)
- we should just explicitly not support (via warning or something) double re-exports of models until
modelNamegoes away entirely. But not try to be clever about fixing?
runspired
force-pushed
the
feat/warn-on-reexports
branch
from
d6c2e07 to
586dba7
Compare
|
I believe this is what folks are hitting in #6143 |
runspired
added
🏷️ cleanup
@bmac If good this should be backported to release and beta.
This twiddle demonstrates the issue: https://ember-twiddle.com/035a072e1818f48c09ac9a38f13b1773?openFiles=twiddle.json%2C
All Ember versions prior to 2.12 work with all ED versions. Beginning in 2.12 (I think we fussed with double extend) this begins breaking in ED.
We can temporarily fix this with a deprecation by extending the class ourselves.