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

Conversation

Projects
None yet
6 participants
@dgeb
Member

dgeb commented Jul 23, 2015

This change represents the final brick needed to wall off the
Container as fully private.

The Container no longer injects itself into every object that it looks
up. Instead, the owner of the Container is injected as owner by the
ContainerProxy mixin.

The current net effect is that an app instance is injected into every
looked up object instead of that app instance's container. This
provides clean, public access to methods exposed by the app
instance's ContainerProxy and RegistryProxy methods. It also guarantees
that the only supported path to get to a Container or Registry is
through a proxied method. This guarantee is important because it allows
for owner-specific logic to be placed in proxy methods.

In the future, other classes, such as Engine (coming soon), may mix in
ContainerProxy and thus have the potential to be "owners".

Note 1: This work is behind the ember-registry-container-reform flag.

Note 2: Totally open to alternatives to the name owner. I was drawn
to this name because the owner of the container "owns" the instantiated
objects to the extent that its responsible for their cleanup.

Note 3: This is still a WIP and will require changes throughout the
test suite for compatibility.


  • Move owner to __owner__ (actually, use symbol instead).
  • Create getOwner / setOwner functions.
  • Add simple container tests for accessing container in an object looked up from the container.
  • Confirm the Object.defineProperty supporting deprecated access to this.container is called a single time per factory (and not once for every created instance).
  • Add a new feature flag (because ember-container-registry-reform is already enabled). Perhaps ember-container-inject-owner?
@stefanpenner

View changes

Show outdated Hide outdated packages/ember-runtime/lib/mixins/container_proxy.js Outdated
@stefanpenner

View changes

Show outdated Hide outdated packages/ember-runtime/lib/mixins/container_proxy.js Outdated
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jul 23, 2015

Member

The issue of container injections was raised by @krisselden.

As I replied, I didn't include this in #11440 because it wasn't part of my original RFC and so I thought it appropriate to discuss it separately.

If feedback is positive, I'll bring the tests / implementation inline.

Member

dgeb commented Jul 23, 2015

The issue of container injections was raised by @krisselden.

As I replied, I didn't include this in #11440 because it wasn't part of my original RFC and so I thought it appropriate to discuss it separately.

If feedback is positive, I'll bring the tests / implementation inline.

@rwjblue

View changes

Show outdated Hide outdated packages/container/lib/container.js Outdated
@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jul 23, 2015

Member

@dgeb owner still is too much of a reserved word, why not __container__ too?

Member

krisselden commented Jul 23, 2015

@dgeb owner still is too much of a reserved word, why not __container__ too?

@krisselden

This comment has been minimized.

Show comment
Hide comment
@krisselden

krisselden Jul 23, 2015

Member

We can have a public API via an accessor that knows the symbol, import getOwner from 'ember-injections' getOwner(this).lookup() and we can have Ember.injectService etc. understand this for lazy injections.

Member

krisselden commented Jul 23, 2015

We can have a public API via an accessor that knows the symbol, import getOwner from 'ember-injections' getOwner(this).lookup() and we can have Ember.injectService etc. understand this for lazy injections.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Jul 24, 2015

Member

owner still is too much of a reserved word, why not container too?

My issue with using __container__ or container is that the property is not in fact an instanceof Container.

We can have a public API via an accessor that knows the symbol, import getOwner from 'ember-injections' getOwner(this).lookup() and we can have Ember.injectService etc. understand this for lazy injections.

Seems good to me.

Member

dgeb commented Jul 24, 2015

owner still is too much of a reserved word, why not container too?

My issue with using __container__ or container is that the property is not in fact an instanceof Container.

We can have a public API via an accessor that knows the symbol, import getOwner from 'ember-injections' getOwner(this).lookup() and we can have Ember.injectService etc. understand this for lazy injections.

Seems good to me.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 9, 2015

Member

Where are we at with this?

Member

rwjblue commented Aug 9, 2015

Where are we at with this?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Aug 12, 2015

Member

Sorry to let this sit.

Would __owner__ work for everyone, combined with the getOwner accessor that @krisselden suggested?

^ @stefanpenner @rwjblue

Member

dgeb commented Aug 12, 2015

Sorry to let this sit.

Would __owner__ work for everyone, combined with the getOwner accessor that @krisselden suggested?

^ @stefanpenner @rwjblue

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 16, 2015

Member

@dgeb - Yeah, seems like a good path forward for now (we can always do more bikeshedding :trollface: ). I think this will need to use a different feature flag name though, as we are going to ship the container / registry reform feature before this is ready.

Member

rwjblue commented Aug 16, 2015

@dgeb - Yeah, seems like a good path forward for now (we can always do more bikeshedding :trollface: ). I think this will need to use a different feature flag name though, as we are going to ship the container / registry reform feature before this is ready.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Aug 23, 2015

Member

@dgeb - I believe we are generally in agreement with your last comment.

General checklist added to PR description.

Member

rwjblue commented Aug 23, 2015

@dgeb - I believe we are generally in agreement with your last comment.

General checklist added to PR description.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Sep 3, 2015

Member

@rwjblue - thanks - I'll try to get to this soon!

Member

dgeb commented Sep 3, 2015

@rwjblue - thanks - I'll try to get to this soon!

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Oct 22, 2015

Member

I've completed all the tasks in the checklist except for the following:

Confirm the Object.defineProperty supporting deprecated access to this.container is called a single time per factory (and not once for every created instance).

If we use Object.defineProperty in injectionsFor instead of instantiate, the defined container property is not in fact applied to the instantiated object. This looks to be because the injections are applied via factory.extend(injections), but more investigation is needed.

Member

dgeb commented Oct 22, 2015

I've completed all the tasks in the checklist except for the following:

Confirm the Object.defineProperty supporting deprecated access to this.container is called a single time per factory (and not once for every created instance).

If we use Object.defineProperty in injectionsFor instead of instantiate, the defined container property is not in fact applied to the instantiated object. This looks to be because the injections are applied via factory.extend(injections), but more investigation is needed.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Oct 22, 2015

Member

Ok, I'm now injecting the container via defineProperty on the generated factory prototype rather than on the injections object (since it wasn't being applied via extend). This injection should only be necessary until Ember 3.0.

The checklist is now complete, but CI is apparently not happy yet.

Member

dgeb commented Oct 22, 2015

Ok, I'm now injecting the container via defineProperty on the generated factory prototype rather than on the injections object (since it wasn't being applied via extend). This injection should only be necessary until Ember 3.0.

The checklist is now complete, but CI is apparently not happy yet.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Oct 23, 2015

Member

Finally green and ready for review :)

Member

dgeb commented Oct 23, 2015

Finally green and ready for review :)

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Oct 30, 2015

Member

@rwjblue - let me know when you're ready to review this and I'll be glad to rebase it.

Member

dgeb commented Oct 30, 2015

@rwjblue - let me know when you're ready to review this and I'll be glad to rebase it.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Oct 30, 2015

Member

Thank you. Sorry for the delay, I've gotten a bit behind on my triaging and PR review while in London for EmberCamp London.

Member

rwjblue commented Oct 30, 2015

Thank you. Sorry for the delay, I've gotten a bit behind on my triaging and PR review while in London for EmberCamp London.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member
packages/container
  • Can you remove @since tags (the earliest this can be enabled is 2.3 assuming we enable right away)? We can re-add the @since flags once we enable the feature and are reasonably certain what version it will land in.
  • In the instantiate function in the Container, it seems that this.container will no longer be present in init (since injectDeprecatedContainer will be called after factory.create). We need to ensure that this.container is present in init (otherwise it is a fairly breaking change). My suggestion would be to grab the result of injectionsFor(container, fullName) into a variable and set container just before calling .create.
  • Should we make injectDeprecatedContainer use return getOwner(this) instead of passing the container in?
  • Should new Container() throw an error (since owner wasn't provided)?
Member

rwjblue commented Nov 2, 2015

packages/container
  • Can you remove @since tags (the earliest this can be enabled is 2.3 assuming we enable right away)? We can re-add the @since flags once we enable the feature and are reasonably certain what version it will land in.
  • In the instantiate function in the Container, it seems that this.container will no longer be present in init (since injectDeprecatedContainer will be called after factory.create). We need to ensure that this.container is present in init (otherwise it is a fairly breaking change). My suggestion would be to grab the result of injectionsFor(container, fullName) into a variable and set container just before calling .create.
  • Should we make injectDeprecatedContainer use return getOwner(this) instead of passing the container in?
  • Should new Container() throw an error (since owner wasn't provided)?
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

packages/ember-runtime

  • getOwner is not exposed alongside Ember.Registry and Ember.Container. Shouldn't it be exposed when the feature is enabled?
Member

rwjblue commented Nov 2, 2015

packages/ember-runtime

  • getOwner is not exposed alongside Ember.Registry and Ember.Container. Shouldn't it be exposed when the feature is enabled?
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

packages/ember-views

  • LGTM

package/ember-htmlbars

  • packages/ember-htmlbars/lib/node-managers/component-node-manager.js in createComponent, it looks like setOwner is conditionally called after init, can we set it on props before calling .create (and therefore remove the need to check again .create)? (it seems that packages/ember-htmlbars/lib/node-managers/view-node-manager.js does this already)
Member

rwjblue commented Nov 2, 2015

packages/ember-views

  • LGTM

package/ember-htmlbars

  • packages/ember-htmlbars/lib/node-managers/component-node-manager.js in createComponent, it looks like setOwner is conditionally called after init, can we set it on props before calling .create (and therefore remove the need to check again .create)? (it seems that packages/ember-htmlbars/lib/node-managers/view-node-manager.js does this already)
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 2, 2015

Member

Can you remove @SInCE tags (the earliest this can be enabled is 2.3 assuming we enable right away)?

Sure thing - I removed the one I found: Container#owner. Please let me know if there are others.

In the instantiate function in the Container, it seems that this.container will no longer be present in init (since injectDeprecatedContainer will be called after factory.create). We need to ensure that this.container is present in init (otherwise it is a fairly breaking change). My suggestion would be to grab the result of injectionsFor(container, fullName) into a variable and set container just before calling .create.

I see what you're saying regarding the scenario in which ember-container-inject-owner is enabled. I think the answer might be to always set injections.container = container; in injectionsFor and then override that in instantiate with injectDeprecatedContainer.

Should we make injectDeprecatedContainer use return getOwner(this) instead of passing the container in?

This would be a breaking change, since the interface for ContainerProxy doesn't align perfectly with that of Container (we privatized _lookupFactory for instance). I think this will cause more trouble than its worth.

Should new Container() throw an error (since owner wasn't provided)?

I guess this depends how general-purpose we want to keep the Container and Registry classes. I could see Container being used generically in an application that's not concerned injecting an owner.

getOwner is not exposed alongside Ember.Registry and Ember.Container. Shouldn't it be exposed when the feature is enabled?

Good call. I just exposed Ember.getOwner and Ember.setOwner (no harm in exposing them now, right?).

Member

dgeb commented Nov 2, 2015

Can you remove @SInCE tags (the earliest this can be enabled is 2.3 assuming we enable right away)?

Sure thing - I removed the one I found: Container#owner. Please let me know if there are others.

In the instantiate function in the Container, it seems that this.container will no longer be present in init (since injectDeprecatedContainer will be called after factory.create). We need to ensure that this.container is present in init (otherwise it is a fairly breaking change). My suggestion would be to grab the result of injectionsFor(container, fullName) into a variable and set container just before calling .create.

I see what you're saying regarding the scenario in which ember-container-inject-owner is enabled. I think the answer might be to always set injections.container = container; in injectionsFor and then override that in instantiate with injectDeprecatedContainer.

Should we make injectDeprecatedContainer use return getOwner(this) instead of passing the container in?

This would be a breaking change, since the interface for ContainerProxy doesn't align perfectly with that of Container (we privatized _lookupFactory for instance). I think this will cause more trouble than its worth.

Should new Container() throw an error (since owner wasn't provided)?

I guess this depends how general-purpose we want to keep the Container and Registry classes. I could see Container being used generically in an application that's not concerned injecting an owner.

getOwner is not exposed alongside Ember.Registry and Ember.Container. Shouldn't it be exposed when the feature is enabled?

Good call. I just exposed Ember.getOwner and Ember.setOwner (no harm in exposing them now, right?).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

I see what you're saying regarding the scenario in which ember-container-inject-owner is enabled. I think the answer might be to always set injections.container = container; in injectionsFor and then override that in instantiate with injectDeprecatedContainer.

I just dug a tad bit more, and it seems like https://github.com/dgeb/ember.js/blob/inject-owner-not-container/packages/container/lib/container.js#L276-L278 resolves my concern. Both container and the owner symbol will be set properly in init.

Member

rwjblue commented Nov 2, 2015

I see what you're saying regarding the scenario in which ember-container-inject-owner is enabled. I think the answer might be to always set injections.container = container; in injectionsFor and then override that in instantiate with injectDeprecatedContainer.

I just dug a tad bit more, and it seems like https://github.com/dgeb/ember.js/blob/inject-owner-not-container/packages/container/lib/container.js#L276-L278 resolves my concern. Both container and the owner symbol will be set properly in init.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

This would be a breaking change, since the interface for ContainerProxy doesn't align perfectly with that of Container (we privatized _lookupFactory for instance). I think this will cause more trouble than its worth.

👍 - Agreed. I liked the idea of using the same public API, but you are right it is breaking

Member

rwjblue commented Nov 2, 2015

This would be a breaking change, since the interface for ContainerProxy doesn't align perfectly with that of Container (we privatized _lookupFactory for instance). I think this will cause more trouble than its worth.

👍 - Agreed. I liked the idea of using the same public API, but you are right it is breaking

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

Good call. I just exposed Ember.getOwner and Ember.setOwner (no harm in exposing them now, right?).

Lets only expose them if the feature is enabled.

Member

rwjblue commented Nov 2, 2015

Good call. I just exposed Ember.getOwner and Ember.setOwner (no harm in exposing them now, right?).

Lets only expose them if the feature is enabled.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

packages/ember-extension-support

  • We need to confirm that the removal of container prop in packages/ember-extension-support/lib/container_debug_adapter.js and packages/ember-extension-support/lib/data_adapter.js isn't an issue in the inspector (I doubt that it is, but I'd like to confirm)
Member

rwjblue commented Nov 2, 2015

packages/ember-extension-support

  • We need to confirm that the removal of container prop in packages/ember-extension-support/lib/container_debug_adapter.js and packages/ember-extension-support/lib/data_adapter.js isn't an issue in the inspector (I doubt that it is, but I'd like to confirm)
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

I believe that @dgeb is incorporating that last set of changes/tweaks discussed above, and this will be ready to merge pretty soon.

@stefanpenner / @krisselden: I believe that this satisfies the previous concerns that we all had. Please review if/when you have time. GH doesn't do the diff very well (due to the number of test changes where we were manually creating containers), but here is a gist with the various packages/**lib/ changes.

Member

rwjblue commented Nov 2, 2015

I believe that @dgeb is incorporating that last set of changes/tweaks discussed above, and this will be ready to merge pretty soon.

@stefanpenner / @krisselden: I believe that this satisfies the previous concerns that we all had. Please review if/when you have time. GH doesn't do the diff very well (due to the number of test changes where we were manually creating containers), but here is a gist with the various packages/**lib/ changes.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 2, 2015

Member

I believe that @dgeb is incorporating that last set of changes/tweaks discussed above, and this will be ready to merge pretty soon.

I believe that the only remaining task is to inject a full FakeContainer proxy (created once per container instance) to support deprecated access to the container from within instantiated objects' init methods. @rwjblue and I discussed handling this as a separate PR that must land before the flag is enabled.

Member

dgeb commented Nov 2, 2015

I believe that @dgeb is incorporating that last set of changes/tweaks discussed above, and this will be ready to merge pretty soon.

I believe that the only remaining task is to inject a full FakeContainer proxy (created once per container instance) to support deprecated access to the container from within instantiated objects' init methods. @rwjblue and I discussed handling this as a separate PR that must land before the flag is enabled.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 2, 2015

Member

Confirm.

@dgeb - Can you create an issue for that once this is merged (just so we don't forget)?

Member

rwjblue commented Nov 2, 2015

Confirm.

@dgeb - Can you create an issue for that once this is merged (just so we don't forget)?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 3, 2015

Member

Can you create an issue for that once this is merged (just so we don't forget)?

Sure thing.

Member

dgeb commented Nov 3, 2015

Can you create an issue for that once this is merged (just so we don't forget)?

Sure thing.

obj = factory.create(injectionsFor(container, fullName));
if (isEnabled('ember-container-inject-owner')) {
injectDeprecatedContainer(obj, container);

This comment has been minimized.

@stefanpenner

stefanpenner Nov 3, 2015

Member

doesn't this already happen in injectionsFor ?

@stefanpenner

stefanpenner Nov 3, 2015

Member

doesn't this already happen in injectionsFor ?

This comment has been minimized.

@rwjblue

rwjblue Nov 3, 2015

Member

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).

@rwjblue

rwjblue Nov 3, 2015

Member

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).

This comment has been minimized.

@stefanpenner

stefanpenner Nov 3, 2015

Member

ah ok, the fakeContainer approach makes sense.

@stefanpenner

stefanpenner Nov 3, 2015

Member

ah ok, the fakeContainer approach makes sense.

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

This comment has been minimized.

@stefanpenner

stefanpenner Nov 3, 2015

Member

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?

@stefanpenner

stefanpenner Nov 3, 2015

Member

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?

@stefanpenner

View changes

Show outdated Hide outdated packages/container/tests/container_test.js Outdated
@stefanpenner

View changes

Show outdated Hide outdated packages/container/tests/container_test.js Outdated
@stefanpenner

View changes

Show outdated Hide outdated packages/container/tests/owner_test.js Outdated
@stefanpenner

View changes

Show outdated Hide outdated packages/container/tests/owner_test.js Outdated
setOwner(obj, owner);
strictEqual(getOwner(obj), owner, 'owner has been set');

This comment has been minimized.

@stefanpenner

stefanpenner Nov 3, 2015

Member

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

@stefanpenner

stefanpenner Nov 3, 2015

Member

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

// for the fallback case
component.container = component.container || env.container;
if (!getOwner(component)) {

This comment has been minimized.

@stefanpenner

stefanpenner Nov 3, 2015

Member

This is basically in few scenarios correct? We should be sure in nearly all cases owner is already set.

@stefanpenner

stefanpenner Nov 3, 2015

Member

This is basically in few scenarios correct? We should be sure in nearly all cases owner is already set.

This comment has been minimized.

@rwjblue

rwjblue Nov 3, 2015

Member

Yes, it should essentially always be set already

@rwjblue

rwjblue Nov 3, 2015

Member

Yes, it should essentially always be set already

@stefanpenner

View changes

Show outdated Hide outdated packages/ember-htmlbars/tests/compat/view_helper_test.js Outdated
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 3, 2015

Member

the setOwner usage in tests seems unfortunate. The entity should be initialized with its reference to the owner, it seems perfectly reasonable for one to assume in all cases the owner is present during view init.

I suspect, something like:

View.extend(ownerInject(), {

}).create()

would do the trick

Member

stefanpenner commented Nov 3, 2015

the setOwner usage in tests seems unfortunate. The entity should be initialized with its reference to the owner, it seems perfectly reasonable for one to assume in all cases the owner is present during view init.

I suspect, something like:

View.extend(ownerInject(), {

}).create()

would do the trick

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 3, 2015

Member

@stefanpenner

I'm open to using a helper to clean up the tests. Keep in mind we need to pass owner into whatever helper we choose.

For example:

View.extend(ownerInject(owner), {
  prop: value
}).create()

Alternative names: injectOwner or ownedBy

Another possible helper:

View.extend(injectOwner({
  prop: value
}, owner)).create()

Alternative name: withOwner

@stefanpenner @rwjblue any preference?

Member

dgeb commented Nov 3, 2015

@stefanpenner

I'm open to using a helper to clean up the tests. Keep in mind we need to pass owner into whatever helper we choose.

For example:

View.extend(ownerInject(owner), {
  prop: value
}).create()

Alternative names: injectOwner or ownedBy

Another possible helper:

View.extend(injectOwner({
  prop: value
}, owner)).create()

Alternative name: withOwner

@stefanpenner @rwjblue any preference?

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 3, 2015

Member

After chatting with @rwjblue, we're looking to export the OWNER symbol for use by Ember and its tests, but we won't be exposing it publicly (on the Ember global).

This will allow us to set the owner without a helper in Ember's tests:

View.extend({
  [OWNER]: owner,
  prop: value
}).create()

@stefanpenner let us know if you have any objections.

Member

dgeb commented Nov 3, 2015

After chatting with @rwjblue, we're looking to export the OWNER symbol for use by Ember and its tests, but we won't be exposing it publicly (on the Ember global).

This will allow us to set the owner without a helper in Ember's tests:

View.extend({
  [OWNER]: owner,
  prop: value
}).create()

@stefanpenner let us know if you have any objections.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 3, 2015

Member

@stefanpenner let us know if you have any objections.

nope this sounds good.

Member

stefanpenner commented Nov 3, 2015

@stefanpenner let us know if you have any objections.

nope this sounds good.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 3, 2015

Member

Once this lands, we will seal a private but often used feature. One for example used heavily by ember-data. Do we have strategies in-place to provide public alternatives?

Member

stefanpenner commented Nov 3, 2015

Once this lands, we will seal a private but often used feature. One for example used heavily by ember-data. Do we have strategies in-place to provide public alternatives?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 4, 2015

Member

Yes, this PR exposes the new public API.

Instead of using this (which is private):

this.container.lookup('foo:bar')

Once this feature is enabled, you would do:

Ember.getOwner(this).lookup('foo:bar')

The only things exposed on the returned owner are the functions that we have already decided are public (during he ember-container-reform feature that landed in 2.1).

Member

rwjblue commented Nov 4, 2015

Yes, this PR exposes the new public API.

Instead of using this (which is private):

this.container.lookup('foo:bar')

Once this feature is enabled, you would do:

Ember.getOwner(this).lookup('foo:bar')

The only things exposed on the returned owner are the functions that we have already decided are public (during he ember-container-reform feature that landed in 2.1).

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 4, 2015

Member

Also, please note: this.container still fully works with a deprecation (since it is SUPER heavily used).

Member

rwjblue commented Nov 4, 2015

Also, please note: this.container still fully works with a deprecation (since it is SUPER heavily used).

dgeb added some commits Oct 16, 2015

[FEATURE ember-container-inject-owner] Introduce getOwner / setOwner …
…helpers.

Use a symbol to obscure the owner property.
[FEATURE ember-container-inject-owner] Inject `owner` instead of `con…
…tainer` during `lookup`.

This change represents the final brick needed to wall off the
`Container` as fully private.

The Container no longer injects itself into every object that it looks
up. Instead, it uses the new `setOwner` helper to inject the "owner",
which can then be retrieved from any object using the new `getOwner`
helper.

The current net effect is that an app instance is injected into every
looked up object instead of that app instance's container. This
provides clean, public access to methods exposed by the app
instance's ContainerProxy and RegistryProxy methods. It also guarantees
that the only supported path to get to a Container or Registry is
through a proxied method. This guarantee is important because it allows
for owner-specific logic to be placed in proxy methods.

In the future, other classes, such as Engine (coming soon), may mix in
ContainerProxy and thus have the potential to be "owners".

This work is behind the `ember-container-inject-owner` flag. Without
this flag enabled, the Container will continue to inject itself directly
into objects that it instantiates (as `container`).
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Nov 4, 2015

Member

Ok - I've applied the changes discussed above and all is green (except for the sauce build). Let me know if there's anything else I can do to help this land.

Member

dgeb commented Nov 4, 2015

Ok - I've applied the changes discussed above and all is green (except for the sauce build). Let me know if there's anything else I can do to help this land.

@rwjblue rwjblue changed the title from [FEATURE ember-registry-container-reform] WIP: Inject `owner` instead of `container` during `lookup`. to [FEATURE ember-container-inject-owner]: Inject `owner` instead of `container` during `lookup`. Nov 4, 2015

@rwjblue rwjblue referenced this pull request Nov 4, 2015

Closed

ember-container-inject-owner - Checklist #12555

8 of 8 tasks complete

rwjblue added a commit that referenced this pull request Nov 4, 2015

Merge pull request #11874 from dgeb/inject-owner-not-container
[FEATURE ember-registry-container-reform] WIP: Inject `owner` instead of `container` during `lookup`.

@rwjblue rwjblue merged commit cb93860 into emberjs:master Nov 4, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 4, 2015

Member

#12555 is a checklist of items needed to be done for enabling the new feature.

Member

rwjblue commented Nov 4, 2015

#12555 is a checklist of items needed to be done for enabling the new feature.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Nov 4, 2015

Member

@dgeb sounds good

Member

stefanpenner commented Nov 4, 2015

@dgeb sounds good

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Nov 20, 2015

Contributor

@rwjblue @dgeb et al. -- OMG. Thank you. I just replaced all of my usage of container.XXX() and registry.XXX() with getOwner (via the polyfill) and their new respective API calls. Great to finally have a public API for these things. 👍 🍻

Contributor

workmanw commented Nov 20, 2015

@rwjblue @dgeb et al. -- OMG. Thank you. I just replaced all of my usage of container.XXX() and registry.XXX() with getOwner (via the polyfill) and their new respective API calls. Great to finally have a public API for these things. 👍 🍻

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Nov 20, 2015

Member

Awesome!

Member

rwjblue commented Nov 20, 2015

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment