Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Fixes #4807] realize class + factory seperation #4810

Merged
merged 1 commit into from
Feb 22, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion addon/-private/system/model/internal-model.js
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,7 @@ export default class InternalModel {
createOptions.container = this.store.container;
}

this._record = this.modelClass._create(createOptions);
this._record = this.store.modelFactoryFor(this.modelName).create(createOptions);

this._triggerDeferredTriggers();
heimdall.stop(token);
Expand Down
29 changes: 14 additions & 15 deletions addon/-private/system/model/model.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import {
relatedTypesDescriptor,
relationshipsDescriptor
} from 'ember-data/-private/system/relationships/ext';
import { runInDebug } from 'ember-data/-private/debug';

const {
get,
Expand All @@ -21,7 +22,6 @@ const {
@module ember-data
*/


function findPossibleInverses(type, inverseType, name, relationshipsSoFar) {
let possibleRelationships = relationshipsSoFar || [];

Expand Down Expand Up @@ -1173,17 +1173,20 @@ Object.defineProperty(Model.prototype, 'data', {
}
});

Model.reopenClass({
/**
Alias DS.Model's `create` method to `_create`. This allows us to create DS.Model
instances from within the store, but if end users accidentally call `create()`
(instead of `createRecord()`), we can raise an error.
runInDebug(function() {
Model.reopen({
init() {
this._super(...arguments);

@method _create
@private
@static
*/
_create: Model.create,
if (!this._internalModel) {
throw new Ember.Error('You should not call `create` on a model. Instead, call `store.createRecord` with the attributes you would like to set.');
}
}
});
});

Model.reopenClass({
isModel: true,

/**
Override the class' `create()` method to raise an error. This
Expand All @@ -1196,10 +1199,6 @@ Model.reopenClass({
@private
@static
*/
create() {
throw new Ember.Error("You should not call `create` on a model. Instead, call `store.createRecord` with the attributes you would like to set.");
},

/**
Represents the model's class name as a string. This can be used to look up the model through
DS.Store's modelFor method.
Expand Down
23 changes: 15 additions & 8 deletions addon/-private/system/store.js
Original file line number Diff line number Diff line change
Expand Up @@ -2087,6 +2087,12 @@ Store = Service.extend({
@private
*/
_modelFor(modelName) {
let maybeFactory = this._modelFactoryFor(modelName);
// for factorFor factory/class split
return maybeFactory.class ? maybeFactory.class : maybeFactory;
},

_modelFactoryFor(modelName) {
heimdall.increment(modelFor);
let factory = this._modelClassCache[modelName];

Expand All @@ -2101,9 +2107,13 @@ Store = Service.extend({
throw new EmberError(`No model was found for '${modelName}'`);
}

assert(`'${inspect(factory)}' does not appear to be an ember-data model`, (typeof factory._create === 'function') );
// interopt with the future
let klass = getOwner(this).factoryFor ? factory.class : factory;

assert(`'${inspect(klass)}' does not appear to be an ember-data model`, klass.isModel);

factory.modelName = factory.modelName || modelName;
// TODO: deprecate this
klass.modelName = klass.modelName || modelName;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you do Ember.deprecate instead of the TODO?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't currently have a path forward yet. I would prefer to deprecate once this is the case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But will find a path forward for this soon!

Copy link
Member Author

@stefanpenner stefanpenner Feb 21, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@chadhietala I'm doing a follow up PR (right now) that reduces ember-datas dependence on modelName. That will be a pre-cursor to deprecating it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SG

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i've removed many more internal usages of modelName, there are some more low hanging fruit but addressing it fully will be some effort (although I hope we can do asap).

Some notes: #4814


this._modelClassCache[modelName] = factory;
}
Expand All @@ -2116,16 +2126,13 @@ Store = Service.extend({
*/
modelFactoryFor(modelName) {
heimdall.increment(modelFactoryFor);
assert("You need to pass a model name to the store's modelFactoryFor method", isPresent(modelName));
assert('Passing classes to store methods has been removed. Please pass a dasherized string instead of '+ inspect(modelName), typeof modelName === 'string');
assert(`You need to pass a model name to the store's modelFactoryFor method`, isPresent(modelName));
assert(`Passing classes to store methods has been removed. Please pass a dasherized string instead of ${modelName}`, typeof modelName === 'string');
let trueModelName = this._classKeyFor(modelName);
let owner = getOwner(this);

if (owner.factoryFor) {
let MaybeModel = owner.factoryFor(`model:${trueModelName}`);
let MaybeModelFactory = MaybeModel && MaybeModel.class;

return MaybeModelFactory;
return owner.factoryFor(`model:${trueModelName}`);
} else {
return owner._lookupFactory(`model:${trueModelName}`);
}
Expand Down
76 changes: 76 additions & 0 deletions tests/integration/injection-test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import setupStore from 'dummy/tests/helpers/store';
import Ember from 'ember';
import DS from 'ember-data';
import { module, test } from 'qunit';

let env, originalFactoryFor, originalMODEL_FACTORY_INJECTIONS = Ember.MODEL_FACTORY_INJECTIONS;
const { run } = Ember;

const factory = {
isFactory: true,
class: {
isModel: true,
_create() { }
}
};

module('integration/injection factoryFor enabled', {
setup() {
env = setupStore();

originalFactoryFor = Ember.getOwner(env.store).factoryFor;

Ember.getOwner(env.store).factoryFor = function(name) {
return factory;
};
},

teardown() {
Ember.getOwner(env.store).factoryFor = originalFactoryFor;

run(env.store, 'destroy');
}
});

test('modelFactoryFor', function(assert) {
const modelFactory = env.store.modelFactoryFor('super-villain');

assert.equal(modelFactory, factory, 'expected the factory itself to be returned');
});

test('modelFor', function(assert) {
const modelFactory = env.store.modelFor('super-villain');

assert.equal(modelFactory, factory.class, 'expected the factory itself to be returned');

// TODO: we should deprecate this next line. Resolved state on the class is fraught with peril
assert.equal(modelFactory.modelName, 'super-villain', 'expected the factory itself to be returned');
});

module('integration/injection eager injections', {
setup() {
Ember.MODEL_FACTORY_INJECTIONS = true;
env = setupStore();

env.registry.injection('model:foo', 'apple', 'service:apple');
env.registry.register('model:foo', DS.Model);
env.registry.register('service:apple', Ember.Object.extend({ isService: true }));
// container injection
},

teardown() {
// can be removed once we no longer support ember versions without lookupFactory
Ember.MODEL_FACTORY_INJECTIONS = originalMODEL_FACTORY_INJECTIONS;

run(env.store, 'destroy');
}
});

test('did inject', function(assert) {
let foo = run(() => env.store.createRecord('foo'));
let apple = foo.get('apple');
let Apple = env.registry.registrations['service:apple'];

assert.ok(apple, `'model:foo' instance should have an 'apple' property`);
assert.ok(apple instanceof Apple, `'model:foo'.apple should be an instance of 'service:apple'`);
});
2 changes: 1 addition & 1 deletion tests/unit/model-test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ var run = Ember.run;

var Person, store, env;

module("unit/model - DS.Model", {
module('unit/model - DS.Model', {
beforeEach() {
Person = DS.Model.extend({
name: DS.attr('string'),
Expand Down
Loading