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

[FEATURE ember-container-inject-owner]: Inject owner instead of container during lookup. #11874

Merged
merged 2 commits into from
Nov 4, 2015
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
3 changes: 2 additions & 1 deletion features.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
"ember-debug-handlers": true,
"ember-routing-routable-components": null,
"ember-metal-ember-assign": null,
"ember-contextual-components": true
"ember-contextual-components": true,
"ember-container-inject-owner": null
}
}
50 changes: 47 additions & 3 deletions packages/container/lib/container.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import Ember from 'ember-metal/core';
import { assert } from 'ember-metal/debug';
import dictionary from 'ember-metal/dictionary';
import isEnabled from 'ember-metal/features';
import { setOwner } from './owner';

/**
A container used to instantiate and cache objects.
Expand All @@ -17,12 +19,20 @@ import dictionary from 'ember-metal/dictionary';
*/
function Container(registry, options) {
this.registry = registry;
this.owner = options && options.owner ? options.owner : null;
this.cache = dictionary(options && options.cache ? options.cache : null);
this.factoryCache = dictionary(options && options.factoryCache ? options.factoryCache : null);
this.validationCache = dictionary(options && options.validationCache ? options.validationCache : null);
}

Container.prototype = {
/**
@private
@property owner
@type Object
*/
owner: null,

/**
@private
@property registry
Expand Down Expand Up @@ -232,6 +242,11 @@ function factoryFor(container, fullName) {
factoryInjections._toString = registry.makeToString(factory, fullName);

var injectedFactory = factory.extend(injections);

if (isEnabled('ember-container-inject-owner')) {
injectDeprecatedContainer(injectedFactory.prototype, container);
}

injectedFactory.reopenClass(factoryInjections);

if (factory && typeof factory._onLookup === 'function') {
Expand All @@ -255,7 +270,14 @@ function injectionsFor(container, fullName) {
registry.getTypeInjections(type),
registry.getInjections(fullName));
injections._debugContainerKey = fullName;
injections.container = container;

setOwner(injections, container.owner);

// TODO - Inject a `FakeContainer` instead here. The `FakeContainer` will
Copy link
Member

Choose a reason for hiding this comment

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

is this TODO intended for this PR or a future one?

Copy link
Member

Choose a reason for hiding this comment

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

Future PR

// proxy all properties of the container with deprecations.
if (!isEnabled('ember-container-inject-owner')) {
injections.container = container;
Copy link
Member

Choose a reason for hiding this comment

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

should this occur in buildInjections?

Copy link
Member

Choose a reason for hiding this comment

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

It has been in injectionsFor to date (see injections.container = container; line being removed a few lines above).

}

return injections;
}
Expand Down Expand Up @@ -299,18 +321,40 @@ function instantiate(container, fullName) {

validationCache[fullName] = true;

let obj;

if (typeof factory.extend === 'function') {
// assume the factory was extendable and is already injected
return factory.create();
obj = factory.create();
} else {
// assume the factory was extendable
// to create time injections
// TODO: support new'ing for instantiation and merge injections for pure JS Functions
return factory.create(injectionsFor(container, fullName));
obj = factory.create(injectionsFor(container, fullName));

if (isEnabled('ember-container-inject-owner')) {
injectDeprecatedContainer(obj, container);
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this already happen in injectionsFor ?

Copy link
Member

Choose a reason for hiding this comment

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

No, we provide the actual (non deprecated) container in injectionsFor, then change it to the deprecated one. This unfortunately means that when the feature is enabled any non Ember Objects instantiated by the container (since this is in the else clause here) will receive the non-deprecated container during their create call, then get .container swapped out to the deprecated one after instantiation.

The solution to this is to create a complete FakeContainer (as mentioned in a comment above and in some of the PR comments). That change is definitely a blocker to this feature being enabled, but I would like to do it as a separate (and smaller PR).

Copy link
Member

Choose a reason for hiding this comment

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

ah ok, the fakeContainer approach makes sense.

}
}

return obj;
}
}

// TODO - remove when Ember reaches v3.0.0
function injectDeprecatedContainer(object, container) {
Object.defineProperty(object, 'container', {
Copy link
Member

Choose a reason for hiding this comment

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

we could likely install the container in injections via a symbol, then leave this defProp on CoreObject, that way all ember objects injections don't need to do the redefine. (only those that are not decedents of CoreObject would?

configurable: true,
enumerable: false,
get() {
Ember.deprecate('Using the injected `container` is deprecated. Please use the `getOwner` helper instead to access the owner of this object.',
false,
{ id: 'ember-application.injected-container', until: '3.0.0' });
return container;
}
});
}

function eachDestroyable(container, callback) {
var cache = container.cache;
var keys = Object.keys(cache);
Expand Down
3 changes: 2 additions & 1 deletion packages/container/lib/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,5 +20,6 @@ if (Ember.ENV && typeof Ember.ENV.MODEL_FACTORY_INJECTIONS !== 'undefined') {

import Registry from 'container/registry';
import Container from 'container/container';
import { getOwner, setOwner } from 'container/owner';

export { Registry, Container };
export { Registry, Container, getOwner, setOwner };
11 changes: 11 additions & 0 deletions packages/container/lib/owner.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import { symbol } from 'ember-metal/utils';

export const OWNER = symbol('OWNER');

export function getOwner(object) {
return object[OWNER];
}

export function setOwner(object, owner) {
object[OWNER] = owner;
}
51 changes: 31 additions & 20 deletions packages/container/tests/container_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import Ember from 'ember-metal/core';
import Registry from 'container/registry';
import { factory } from 'container/tests/container_helper';
import factory from 'container/tests/test-helpers/factory';
import isEnabled from 'ember-metal/features';

var originalModelInjections;

Expand Down Expand Up @@ -57,8 +58,6 @@ QUnit.test('A factory returned from lookupFactory has a debugkey', function() {

registry.register('controller:post', PostController);
var PostFactory = container.lookupFactory('controller:post');

ok(!PostFactory.container, 'factory instance receives a container');
equal(PostFactory._debugContainerKey, 'controller:post', 'factory instance receives _debugContainerKey');
});

Expand All @@ -76,8 +75,6 @@ QUnit.test('fallback for to create time injections if factory has no extend', fu

var postController = container.lookup('controller:post');

ok(postController.container, 'instance receives a container');
equal(postController.container, container, 'instance receives the correct container');
equal(postController._debugContainerKey, 'controller:post', 'instance receives _debugContainerKey');
ok(postController.apple instanceof AppleController, 'instance receives an apple of instance AppleController');
});
Expand All @@ -91,7 +88,6 @@ QUnit.test('The descendants of a factory returned from lookupFactory have a cont
registry.register('controller:post', PostController);
instance = container.lookupFactory('controller:post').create();

ok(instance.container, 'factory instance receives a container');
equal(instance._debugContainerKey, 'controller:post', 'factory instance receives _debugContainerKey');

ok(instance instanceof PostController, 'factory instance is instance of factory');
Expand Down Expand Up @@ -120,18 +116,6 @@ QUnit.test('A registered factory returns a fresh instance if singleton: false is
ok(postController4 instanceof PostController, 'All instances are instances of the registered factory');
});

QUnit.test('A container lookup has access to the container', function() {
var registry = new Registry();
var container = registry.container();
var PostController = factory();

registry.register('controller:post', PostController);

var postController = container.lookup('controller:post');

equal(postController.container, container);
});

QUnit.test('A factory type with a registered injection\'s instances receive that injection', function() {
var registry = new Registry();
var container = registry.container();
Expand Down Expand Up @@ -163,10 +147,8 @@ QUnit.test('An individual factory with a registered injection receives the injec
var postController = container.lookup('controller:post');
var store = container.lookup('store:main');

equal(store.container, container);
equal(store._debugContainerKey, 'store:main');

equal(postController.container, container);
equal(postController._debugContainerKey, 'controller:post');
equal(postController.store, store, 'has the correct store injected');
});
Expand Down Expand Up @@ -535,3 +517,32 @@ QUnit.test('Lazy injection validations are cached', function() {
container.lookup('apple:main');
container.lookup('apple:main');
});

if (isEnabled('ember-container-inject-owner')) {
QUnit.test('A deprecated `container` property is appended to every instantiated object', function() {
let registry = new Registry();
let container = registry.container();
let PostController = factory();
registry.register('controller:post', PostController);
let postController = container.lookup('controller:post');

expectDeprecation(function() {
Ember.get(postController, 'container');
}, 'Using the injected `container` is deprecated. Please use the `getOwner` helper instead to access the owner of this object.');

expectDeprecation(function() {
let c = postController.container;
strictEqual(c, container);
}, 'Using the injected `container` is deprecated. Please use the `getOwner` helper instead to access the owner of this object.');
});
} else {
QUnit.test('A `container` property is appended to every instantiated object', function() {
const registry = new Registry();
const container = registry.container();
const PostController = factory();
registry.register('controller:post', PostController);
const postController = container.lookup('controller:post');

strictEqual(postController.container, container, '');
});
}
16 changes: 16 additions & 0 deletions packages/container/tests/owner_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
import { getOwner, setOwner, OWNER } from 'container/owner';

QUnit.module('Owner', {});

QUnit.test('An owner can be set with `setOwner` and retrieved with `getOwner`', function() {
let owner = {};
let obj = {};

strictEqual(getOwner(obj), undefined, 'owner has not been set');

setOwner(obj, owner);

strictEqual(getOwner(obj), owner, 'owner has been set');
Copy link
Member

Choose a reason for hiding this comment

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

should this test also assert the "symbol" nature of this owner property? To ensure it is non-enumerable, and also not collideable as owner


strictEqual(obj[OWNER], owner, 'owner has been set to the OWNER symbol');
});
2 changes: 1 addition & 1 deletion packages/container/tests/registry_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import Ember from 'ember-metal/core';
import { Registry } from 'container';
import { factory } from 'container/tests/container_helper';
import factory from 'container/tests/test-helpers/factory';

var originalModelInjections;

Expand Down
15 changes: 15 additions & 0 deletions packages/container/tests/test-helpers/build-owner.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import EmberObject from 'ember-runtime/system/object';
import Registry from 'container/registry';
import RegistryProxy from 'ember-runtime/mixins/registry_proxy';
import ContainerProxy from 'ember-runtime/mixins/container_proxy';

export default function buildOwner(props) {
let Owner = EmberObject.extend(RegistryProxy, ContainerProxy, {
init() {
this._super(...arguments);
const registry = this.__registry__ = new Registry();
this.__container__ = registry.container({ owner: this });
}
});
return Owner.create(props);
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ var setProperties = function(object, properties) {

var guids = 0;

var factory = function() {
export default function factory() {
/*jshint validthis: true */

var Klass = function(options) {
Expand Down Expand Up @@ -61,9 +61,4 @@ var factory = function() {

return Child;
}
};

export {
factory,
setProperties
};
}
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ let ApplicationInstance = EmberObject.extend(RegistryProxy, ContainerProxy, {
registry.makeToString = applicationRegistry.makeToString;

// Create a per-instance container from the instance's registry
this.__container__ = registry.container();
this.__container__ = registry.container({ owner: this });

// Register this instance in the per-instance registry.
//
Expand Down
11 changes: 0 additions & 11 deletions packages/ember-extension-support/lib/container_debug_adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,17 +49,6 @@ import EmberObject from 'ember-runtime/system/object';
@public
*/
export default EmberObject.extend({
/**
The container of the application being debugged.
This property will be injected
on creation.

@property container
@default null
@public
*/
container: null,

/**
The resolver instance of the application
being debugged. This property will be injected
Expand Down
16 changes: 2 additions & 14 deletions packages/ember-extension-support/lib/data_adapter.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import Namespace from 'ember-runtime/system/namespace';
import EmberObject from 'ember-runtime/system/object';
import { A as emberA } from 'ember-runtime/system/native_array';
import Application from 'ember-application/system/application';
import { getOwner } from 'container/owner';

/**
@module ember
Expand Down Expand Up @@ -59,19 +60,6 @@ export default EmberObject.extend({
this.releaseMethods = emberA();
},

/**
The container of the application being debugged.
This property will be injected
on creation.

@property container
@default null
@since 1.3.0
@public
*/
container: null,


/**
The container-debug-adapter which is used
to list all models.
Expand Down Expand Up @@ -171,7 +159,7 @@ export default EmberObject.extend({

_nameToClass(type) {
if (typeof type === 'string') {
type = this.container.lookupFactory(`model:${type}`);
type = getOwner(this)._lookupFactory(`model:${type}`);
}
return type;
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import EmberController from 'ember-runtime/controllers/controller';
import 'ember-extension-support'; // Must be required to export Ember.ContainerDebugAdapter
import Application from 'ember-application/system/application';

var adapter, App;
var adapter, App, appInstance;


function boot() {
Expand All @@ -19,14 +19,16 @@ QUnit.module('Container Debug Adapter', {
});
boot();
run(function() {
adapter = App.__container__.lookup('container-debug-adapter:main');
appInstance = App.__deprecatedInstance__;
adapter = appInstance.lookup('container-debug-adapter:main');
});
},
teardown() {
run(function() {
adapter.destroy();
appInstance.destroy();
App.destroy();
App = null;
App = appInstance = adapter = null;
});
}
});
Expand Down
2 changes: 1 addition & 1 deletion packages/ember-htmlbars/lib/hooks/component.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ export default function componentHook(renderNode, env, scope, _tagName, params,

let component, layout;
if (isDasherized || !isAngleBracket) {
let result = lookupComponent(env.container, tagName);
let result = lookupComponent(env.owner, tagName);
component = result.component;
layout = result.layout;

Expand Down
Loading