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

Decouple Registry from Container #9981

Merged
merged 18 commits into from Dec 22, 2014

Conversation

Projects
None yet
5 participants
@dgeb
Member

dgeb commented Dec 19, 2014

This PR separates the current Container into stateless and stateful components:

  • Registry - used for stateless factory / option registrations
  • Container - uses a Registry to lookup factories and options, and then instantiates and caches objects itself

This decoupling is one of the infrastructure changes required to support server-side rendering, as outlined in #9938. It allows for multiple containers to reference a common registry, which is key to allowing a single Ember app to support multiple sessions. Other benefits include a cleaner separation of concerns and improved testing performance, since only container state should need to be reset between most tests.

In this first PR, an application is configured with a single registry and a single container. The registry is passed to initializers instead of the container, since initializers should be performing registrations instead of looking up instances.

Subsequent work, which I'm coordinating with @tomdale and @wycats, will involve the introduction of a Session object. A single App will be able to support multiple sessions, each of which will cache data in a single container. A session initialization process will be introduced that parallels the App initialization process.

Backward compatibility

Registry is made backwards compatible by tracking the first instantiated container, and exposing its lookup and lookupFactory methods. Calls to these methods are deprecated, but it's important to allow them to avoid breaking initializers that rely upon them.

Container is made backwards compatible by exposing many of its associated registry's methods. These methods are all deprecated.

Removal of subcontainers

The concept of subcontainers (i.e. parent / children containers) was removed as part of this refactoring. It was a legacy feature that's no longer used within Ember core and has never been publicly documented, so it should be safe to remove without deprecation.

@stefanpenner stefanpenner self-assigned this Dec 19, 2014

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Dec 19, 2014

Member

assigned myself to review after work today

Member

stefanpenner commented Dec 19, 2014

assigned myself to review after work today

@chadhietala

This comment has been minimized.

Show comment
Hide comment
@chadhietala

chadhietala Dec 19, 2014

Member

👍 Thanks @dgeb!

Member

chadhietala commented Dec 19, 2014

👍 Thanks @dgeb!

@logicalhan

This comment has been minimized.

Show comment
Hide comment
@logicalhan

logicalhan commented Dec 19, 2014

yay 👍

@wycats

This comment has been minimized.

Show comment
Hide comment
@wycats

wycats Dec 19, 2014

Member

This is really great! It was awesome chatting with @chadhietala last week about the requirements for this and get it done so quickly. Between this PR and the work @tomdale and I did this week, we're well on our way to a supportable SSR solution!

Member

wycats commented Dec 19, 2014

This is really great! It was awesome chatting with @chadhietala last week about the requirements for this and get it done so quickly. Between this PR and the work @tomdale and I did this week, we're well on our way to a supportable SSR solution!

@stefanpenner

View changes

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

View changes

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

View changes

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

View changes

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

View changes

Show outdated Hide outdated packages/container/lib/registry.js Outdated
@return {function} toString function
*/
makeToString: function(factory, fullName) {
return factory.toString();

This comment has been minimized.

@stefanpenner

stefanpenner Dec 19, 2014

Member

i believe this should return the function, not the string result of invoking toString

@stefanpenner

stefanpenner Dec 19, 2014

Member

i believe this should return the function, not the string result of invoking toString

This comment has been minimized.

@dgeb

dgeb Dec 19, 2014

Member

This is the same implementation of makeToString originally in Container. Certainly doesn't match the comment, does it?

@dgeb

dgeb Dec 19, 2014

Member

This is the same implementation of makeToString originally in Container. Certainly doesn't match the comment, does it?

This comment has been minimized.

@stefanpenner

stefanpenner Dec 20, 2014

Member
@stefanpenner

View changes

Show outdated Hide outdated packages/container/lib/registry.js Outdated
@@ -27,7 +27,7 @@ function verifyNeedsDependencies(controller, container, needs) {
}
// Structure assert to still do verification but not string concat in production
if (!container.has(dependency)) {
if (!container._registry.has(dependency)) {

This comment has been minimized.

@stefanpenner

stefanpenner Dec 19, 2014

Member

reaching into _registry seems bad, can we instead make this a public non-deprecated api on container?

@stefanpenner

stefanpenner Dec 19, 2014

Member

reaching into _registry seems bad, can we instead make this a public non-deprecated api on container?

This comment has been minimized.

@dgeb

dgeb Dec 19, 2014

Member

Agreed it's not good. I originally had the private _registry as the public registry, so it didn't seem so bad. We could either go back to that or expose select methods like _registry.has as has on Container.

@dgeb

dgeb Dec 19, 2014

Member

Agreed it's not good. I originally had the private _registry as the public registry, so it didn't seem so bad. We could either go back to that or expose select methods like _registry.has as has on Container.

This comment has been minimized.

@stefanpenner

stefanpenner Dec 20, 2014

Member

i think has is reasonable api on the container.

@stefanpenner

stefanpenner Dec 20, 2014

Member

i think has is reasonable api on the container.

@@ -149,7 +149,7 @@ export function renderHelper(name, contextString, options) {
var templateName = 'template:' + name;
Ember.assert("You used `{{render '" + name + "'}}`, but '" + name + "' can not be found as either" +
" a template or a view.", container.has("view:" + name) || container.has(templateName) || !!options.fn);
" a template or a view.", container._registry.has("view:" + name) || container._registry.has(templateName) || !!options.fn);

This comment has been minimized.

@stefanpenner

stefanpenner Dec 19, 2014

Member

again seems reachy to not have this as a public api

@stefanpenner

stefanpenner Dec 19, 2014

Member

again seems reachy to not have this as a public api

@@ -324,7 +325,7 @@ test("{{render}} helper should render templates with models multiple times", fun
});
var PostController = EmberObjectController.extend();
container.register('controller:post', PostController, {singleton: false});
container._registry.register('controller:post', PostController, {singleton: false});

This comment has been minimized.

@stefanpenner

stefanpenner Dec 19, 2014

Member

i wonder if we want to consider container.register for tests, to override the registry and the resolve result.

@stefanpenner

stefanpenner Dec 19, 2014

Member

i wonder if we want to consider container.register for tests, to override the registry and the resolve result.

This comment has been minimized.

@dgeb

dgeb Dec 19, 2014

Member

I discussed the need for a session-specific registry that could fall back to the general app registry with @wycats and @tomdale. The rationale is that some usages, like A/B testing, might require a registry per session (in practice this registry would be close to empty). The Session could then expose a register, which would apply to the session-specific registry.

Anyway, I will fix up the tests to avoid accessing container._registry. As I said above, this was sloppy refactoring during the change from container.registry to container._registry.

@dgeb

dgeb Dec 19, 2014

Member

I discussed the need for a session-specific registry that could fall back to the general app registry with @wycats and @tomdale. The rationale is that some usages, like A/B testing, might require a registry per session (in practice this registry would be close to empty). The Session could then expose a register, which would apply to the session-specific registry.

Anyway, I will fix up the tests to avoid accessing container._registry. As I said above, this was sloppy refactoring during the change from container.registry to container._registry.

This comment has been minimized.

@stefanpenner
@stefanpenner

stefanpenner Dec 20, 2014

Member

👍

@stefanpenner

View changes

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

View changes

Show outdated Hide outdated packages/container/lib/container.js Outdated
import Container from 'container/container';
export default Container;
export { Registry, Container };

This comment has been minimized.

@stefanpenner

stefanpenner Dec 20, 2014

Member

should these be separate files? Or is it because of the deprecated functionality they share a lot of helper methods?

@stefanpenner

stefanpenner Dec 20, 2014

Member

should these be separate files? Or is it because of the deprecated functionality they share a lot of helper methods?

This comment has been minimized.

@dgeb

dgeb Dec 20, 2014

Member

I decided to leave Registry in the container package, so I believe main.js for that package should export both Registry and Container. This seemed straightforward, but I'm open to alternatives.

@dgeb

dgeb Dec 20, 2014

Member

I decided to leave Registry in the container package, so I believe main.js for that package should export both Registry and Container. This seemed straightforward, but I'm open to alternatives.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Dec 20, 2014

Member

👍 Great work.

I left some feedback, but all in all this is great.

I suspect many plugin authors will quickly be craving a an alternative to lookup in initializers. I suspect as a future effort we can add some additional life-cycle hooks to correctly facilitate those needs. This seperation changes some ideas I had on the matter, maybe we can noodle about it soon.

Member

stefanpenner commented Dec 20, 2014

👍 Great work.

I left some feedback, but all in all this is great.

I suspect many plugin authors will quickly be craving a an alternative to lookup in initializers. I suspect as a future effort we can add some additional life-cycle hooks to correctly facilitate those needs. This seperation changes some ideas I had on the matter, maybe we can noodle about it soon.

@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Dec 20, 2014

Member

@stefanpenner thanks for all your feedback! I'll try to update soon to avoid a large rebase.

I suspect many plugin authors will quickly be craving a an alternative to lookup in initializers. I suspect as a future effort we can add some additional life-cycle hooks to correctly facilitate those needs. This seperation changes some ideas I had on the matter, maybe we can noodle about it soon.

To be clear: lookup is still supported on registries to avoid breaking initializers. It's hacky because it tracks the first container created by a registry and then forwards lookup calls to it. We can remove this deprecation in Ember 2.0.

As to the future life-cycle hooks, @wycats and I have discussed a SessionInitializer (maybe there's a better name?) that is very similar to a standard Initializer. These kinds of stateful initializers need to be tied in with routing and App instantiation. Very interested to hear your ideas in this area!

Member

dgeb commented Dec 20, 2014

@stefanpenner thanks for all your feedback! I'll try to update soon to avoid a large rebase.

I suspect many plugin authors will quickly be craving a an alternative to lookup in initializers. I suspect as a future effort we can add some additional life-cycle hooks to correctly facilitate those needs. This seperation changes some ideas I had on the matter, maybe we can noodle about it soon.

To be clear: lookup is still supported on registries to avoid breaking initializers. It's hacky because it tracks the first container created by a registry and then forwards lookup calls to it. We can remove this deprecation in Ember 2.0.

As to the future life-cycle hooks, @wycats and I have discussed a SessionInitializer (maybe there's a better name?) that is very similar to a standard Initializer. These kinds of stateful initializers need to be tied in with routing and App instantiation. Very interested to hear your ideas in this area!

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Dec 20, 2014

Member

As to the future life-cycle hooks, @wycats and I have discussed a SessionInitializer (maybe there's a better name?) that is very similar to a standard Initializer. These kinds of stateful initializers need to be tied in with routing and App instantiation. Very interested to hear your ideas in this area!

I don't think this solves the problem, as the problem is the inability to refine an entry managed by the container without artificially looking it up. We likely need the concept of do X when Y is looked up, this allows that natural assembly of the application.

Member

stefanpenner commented Dec 20, 2014

As to the future life-cycle hooks, @wycats and I have discussed a SessionInitializer (maybe there's a better name?) that is very similar to a standard Initializer. These kinds of stateful initializers need to be tied in with routing and App instantiation. Very interested to hear your ideas in this area!

I don't think this solves the problem, as the problem is the inability to refine an entry managed by the container without artificially looking it up. We likely need the concept of do X when Y is looked up, this allows that natural assembly of the application.

dgeb added some commits Dec 9, 2014

Decouple Registry from Container
`Registry` is used for stateless factory / option registrations.

`Container` uses a `Registry` to lookup factories and options, and then
instantiates and caches objects.

This decoupling allows for multiple containers to reference a common
registry, which is key to allowing a single Ember app to support multiple
sessions.

`Container` is made backwards compatible by exposing many of its associated
registry's methods.

The concept of subcontainers (i.e. parent / children containers) was
removed as part of this refactoring. It was a legacy feature that's no
longer used within Ember core and has never been publicly documented,
so it's safe to remove without deprecation.
Remove comments / old tests / TODO related to Container refactor
Questions raised in comments and TODOs will be raised in the PR.
Container constructor signature now takes `registry` as first arg
Also, change `Container#register` to private `Container#_register`
Introduce Registry#container method
This is the preferred method for constructing a Container based on its
associated Registry.

dgeb added some commits Dec 19, 2014

Implement deprecated lookup and lookupFactory on Registry
The first associated container created through registry.container() is
tracked as _defaultContainer. Calls to `lookup` and `lookupFactory` are
then forwarded to this container.

This prevents breaking Ember 1.x initializers that perform more than
registrations in their `initialize` methods.
Improve arguments check in Container#reset
As per @stefanpenner’s concerns, checking `arguments.length` is more 
strict and can prevent a reset of the entire cache if `fullName` is
mistakenly set to a falsey value.

Also, adds docs for `reset`.
Remove items from caches before destroying the items.
If `member.destroy` errors, caches won’t be corrupted.
Prevent re-registration of factories that have been resolved.
An explicit `unregister` should be required to prevent re-registration
of a factory that has been resolved (and therefore probably used to
instantiate objects).
Replace some helper methods with inline implementations.
`addInjection`, `addTypeInjection`, and `initRules` are arguably more
clear inline rather than separate methods that take a large number of
arguments.
Base Container constructor assertion on boolean
Avoid the possibility that `registry` could be a function that could
be called as part of the assertion.
Ensure that registry.unregister() is called prior to register().
Also, clean up indentation inconsistencies.
@dgeb

This comment has been minimized.

Show comment
Hide comment
@dgeb

dgeb Dec 21, 2014

Member

@stefanpenner @wycats

I have just rebased and corrected almost everything discussed above. The exception being that I chose not to expose registry methods such as has on the container without deprecation warnings. I believe that the cases which require this will be better served by being supplied a Session object that encapsulates a session-specific registry (which falls back to the app-specific registry for resolves) and a session-specific container. Therefore, I've left some access to container._register.has (and other methods) with the understanding that a forthcoming PR that introduces the Session object will clean up these code smells.

This one's getting to be a bear to rebase, so thanks in advance for a speedy re-review. I'll be glad to make tweaks ASAP.

Member

dgeb commented Dec 21, 2014

@stefanpenner @wycats

I have just rebased and corrected almost everything discussed above. The exception being that I chose not to expose registry methods such as has on the container without deprecation warnings. I believe that the cases which require this will be better served by being supplied a Session object that encapsulates a session-specific registry (which falls back to the app-specific registry for resolves) and a session-specific container. Therefore, I've left some access to container._register.has (and other methods) with the understanding that a forthcoming PR that introduces the Session object will clean up these code smells.

This one's getting to be a bear to rebase, so thanks in advance for a speedy re-review. I'll be glad to make tweaks ASAP.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Dec 21, 2014

Member

@dgeb awesome thanks, I'll review

Member

stefanpenner commented Dec 21, 2014

@dgeb awesome thanks, I'll review

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner
Member

stefanpenner commented Dec 21, 2014

LGTM

wycats added a commit that referenced this pull request Dec 22, 2014

Merge pull request #9981 from dgeb/container-registry
Decouple Registry from Container

@wycats wycats merged commit 876eecf into emberjs:master Dec 22, 2014

1 check passed

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

fivetanley added a commit to emberjs/data that referenced this pull request Dec 25, 2014

fivetanley added a commit to emberjs/data that referenced this pull request Dec 25, 2014

thaume pushed a commit to thaume/data that referenced this pull request Jan 6, 2015

quiddle added a commit to quiddle/data that referenced this pull request Feb 28, 2015

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