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 fake container with deprecations. #12609

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

dgeb
Copy link
Member

@dgeb dgeb commented Nov 14, 2015

The fake container provides access to the following methods:

  • lookup
  • lookupFactory

These methods are mapped through the equivalent ContainerProxy methods
(lookup and _lookupFactory) on the container's owner.

Deprecation warnings are provided.

TBD: deprecation id and url

@rwjblue
Copy link
Member

rwjblue commented Nov 15, 2015

Let's use the id and URL that was in the deprecation being removed here (I have already linked that url up with a decent deprecation guide entry).

@rwjblue
Copy link
Member

rwjblue commented Nov 15, 2015

Also, I think we should leave the current deprecation (on property getter for 'container'). The fake container will only be used during instantiation of objects that do not have an .extend (as all of those that do have already had the getter added).

@dgeb
Copy link
Member Author

dgeb commented Nov 15, 2015

Sounds good, @rwjblue. I'll make those changes.

Would you also prefer that the FakeContainer not proxy through to the owner's equivalent methods, but instead just proxy through to the container's methods? Otherwise accessing container at different times could theoretically produce different behavior. Not to mention that FakeContainer -> owner -> container is pretty circuitous.

@rwjblue
Copy link
Member

rwjblue commented Nov 15, 2015

Would you also prefer that the FakeContainer not proxy through to the owner's equivalent methods, but instead just proxy through to the container's methods?

Yes, but if the FakeContainer is essentially only exposed externally by this branch.


I think the changes to the PR would be (only noting them because I just traced the various pathways):

  • Remove this conditional and instead always add the FakeContainer there in injectionsFor.
  • For any factories with .extend we will override that FakeContainer injection with the current property getter deprecation here (this is good)
  • For any factories without .extend (or models with MODEL_FACTORY_INJECTIONS off) you would still get the FakeContainer due to the usage of injectionsFor in that instantiate

@@ -535,6 +535,39 @@ if (isEnabled('ember-container-inject-owner')) {
strictEqual(c, container);
}, 'Using the injected `container` is deprecated. Please use the `getOwner` helper instead to access the owner of this object.');
});

QUnit.test('A deprecated `container` property is appended to every object instantiated from a non-extendable factory, and a fake container is available during instantiation.', function() {
let registry = new Registry();
Copy link
Member

Choose a reason for hiding this comment

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

Can you add expect(5) here (just to ensure that all the expectations are firing)?

…extendable factories.

The fake container is required to provide access to the `container`
during instantiation of objects by non-extendable factories.

The fake container provides access to the following methods:

* `lookup`
* `lookupFactory`

Deprecation warnings are provided that direct the user to call
`getOwner` followed by the equivalent `ContainerProxy` methods
(`lookup` and `_lookupFactory`) on the owner.
@dgeb
Copy link
Member Author

dgeb commented Nov 15, 2015

@rwjblue ok, just implemented your suggestions

rwjblue added a commit that referenced this pull request Nov 16, 2015
[FEATURE ember-container-inject-owner] Inject fake container with deprecations.
@rwjblue rwjblue merged commit f434c9f into emberjs:master Nov 16, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants