Container / Registry Reform #11440

dgeb · 2015-06-13T23:51:24Z

This is an implementation of RFC 46. The goal is to fully encapsulate and privatize the Container and Registry classes by exposing a select subset of public methods on Application and ApplicationInstance.

This is achieved by introducing two new proxy mixins: RegistryProxy and ContainerProxy.

Application now mixes in RegistryProxy.

ApplicationInstance mixes in both RegistryProxy and ContainerProxy.

RegistryProxy

RegistryProxy maintains the registry as a private __registry__ property.

The following registry methods are exposed publicly via RegistryProxy:

resolveRegistration - alias for resolve
register
unregister
hasRegistration - alias for has
registerOption - alias for option
registerOptions - alias for options
registeredOptions - alias for getOptions
registerOptionsForType - alias for optionsForType
registeredOptionsForType - alias for getOptionsForType
inject - alias for injection

ContainerProxy

ContainerProxy maintains the container as a private __container__ property.

The following container methods are exposed publicly via ContainerProxy:

lookup
lookupFactory

Privatization of Container and Registry

The introduction of these interfaces allows for the complete privatization of the Container and Registry classes. This allows for their architecture and documentation to be cleaned up. For instance, a Container can freely reference its associated Registry as registry rather than _registry, as it can be assumed that only framework developers will reference this property.

Simplification of Initializers

Application initializers now receive a single argument to initialize: application. Receiving two arguments is still allowed, but deprecated.

Likewise, ApplicationInstance initializers still receive a single argument to initialize: applicationInstance.

Allow Application Instance's Registrations to take Precedence over the Resolver

This work also addresses #11174 by only associating a resolver with the Application's registry and not with ApplicationInstance's registry.

This has a couple benefits:

It allows registrations made in the app instance's registry to be resolved before falling back to the app's registry, rather than hitting the resolver beforehand. This is important for scenarios such as testing in which custom registrations need to take precedence over the resolver.
It is also more efficient in the case when the app's registry's registrations are used to resolve, because it avoids calling the same resolver twice before finally getting the registration.

Remaining Action Items:

@dgeb to rebase and submit a new PR for [CLEANUP beta] Remove deprecated registry behaviour #11756. This will remove all deprecated code paths for 2.0.0.
@dgeb to add feature flags for all new public API to this PR.
@dgeb to add a work around (like mentioned in Container / Registry Reform #11440 (comment)) to ensure that appInstance.container.lookup continues to work.

tomdale · 2015-06-15T20:28:18Z

This looks great. 👍

dgeb · 2015-06-18T14:03:42Z

@stefanpenner ping re: details on your proposal to replace lookupFactory. If it looks good, I'll include an implementation in this PR so we can get this merged.

dgeb · 2015-06-29T11:53:13Z

@stefanpenner ping ping (the dreaded double ping, best employed on Monday mornings).

chadhietala · 2015-06-29T15:23:50Z

packages/ember-application/lib/system/application.js

-        ref(registry, App);
+
+        // TODO2.0 remove deprecated case in which initialize takes two args
+        if (ref.length === 2) {


If this is not going to land in a 1.13.x release, this can be removed I think.

Yes, that is the question. I'd hate to land this post-2.0 and carry forward all these deprecations until 3.0.

dgeb · 2015-07-08T13:26:50Z

The lack of movement on this issue concerns me. If we do nothing, the following will need to be carried forward into all Ember 2.x releases:

Application initializers that accept two arguments: application and registry. This effectively makes Registry public, whether it's recognized as such in the docs. It also leaves ambiguity for developers as to whether to call register and inject on application or registry.
Application instance initializers that accept two arguments: applicationInstance and container. This effectively makes Container public as well. It doesn't provide a clear public path to access the instance-level registry. Some developers go through container._registry, others through applicationInstance.registry - both are marked private.
Treatment of Registry and Container as "de facto" public classes. This means that we need to read the tea leaves of how they're being accessed by developers in practice whenever they are changed.

I'm trying to get attention to this issue now because deprecating the above is messy, and those messy deprecations will need to be carried forward through all 2.x releases. I'd like to avoid a second long-lasting wave of container / registry deprecation warnings like the one experienced after the SSR refactor.

On the other hand, if we establish clear public interfaces for 2.0, we can demystify Ember's DI approach through docs and guides. Even if this approach is tweaked during 2.x (e.g. access to lookupFactory), we can more surgically deprecate the old behaviors because the public interfaces will be clear.

I know time is short now, but please consider this low hanging fruit. Just say the word and I'll rebase ASAP.

Note: this PR also contains a fix to #11174 which should probably be merged regardless of the bigger picture changes.

stefanpenner · 2015-07-08T14:41:31Z

I would like to give this some time. But my plate of awfully full. I'll try to find time yet this week. But my queue is quite saturated

tomdale · 2015-07-08T14:53:01Z

@stefanpenner If we cannot get your feedback on this, can we get this merged in? We are quickly running out of time, @dgeb has put a ton of effort getting it backwards compatible, and both me and @wycats are 👍 on it.

stefanpenner · 2015-07-08T15:37:03Z

@stefanpenner If we cannot get your feedback on this, can we get this merged in?

Sure, but I reserve the right to complain violently down the road

rwjblue · 2015-07-08T16:36:22Z

Sure, but I reserve the right to complain violently down the road

Have no fear, I even complain violently about the PR's that I made.

dgeb · 2015-07-16T17:32:41Z

I made significant progress with a rebase last night, but still have a couple failing builds: https://travis-ci.org/emberjs/ember.js/builds/71200614

Will do another pass of rebasing/fixing today ...

rwjblue · 2015-07-16T17:39:31Z

The old-jquery build was a spurious failure, I restarted and it is passing now.

The node-tests build is a legit failure, you need to update https://github.com/emberjs/ember.js/blob/master/tests/node/app-boot-test.js#L97 to use app.register instead of app.registry.register (I think).

dgeb · 2015-07-16T18:18:09Z

Thanks for the tip @rwjblue - all green now!

I don't know whether you want to merge this or wait until @wycats and I have a chance to revisit lookupFactory.

rwjblue · 2015-07-21T18:40:01Z

@dgeb and I just discussed at length the path forward for this feature. The following is the rough result:

This PR does quite a few things, but the vast majority of the changes are additive and not breaking. This means we can add those features in the 2.x cycle at any time, behind a feature flag.

The primary areas that we identified that were potentially breaking were:

Application initializers will receive a single argument (the application instance with the container proxy mixed in). This PR already includes an arity check for initializer.initialize function. If two arguments are expected, they will be provided properly.
The ApplicationInstance currently exposes the container publicly. This will need a little bit of work, but a potential solution for non-breaking changes would be:

Object.defineProperty(ApplicationInstance, 'container', {
    configurable: true,
    enumerable: false,
    get() {
      var instance = this;
      return {
        lookup() { 
          Ember.deprecate('Using `ApplicationInstance.container.lookup` is deprecated. Please use `ApplicationIntance.lookup` instead.');
          instance.lookup(...arguments);
      }
    }
  });

Both of those scenarios can be completely mitigated when the new feature is enabled.

Remaining action items (also added to description above):

@dgeb to rebase and submit a new PR for [CLEANUP beta] Remove deprecated registry behaviour #11756. This will remove all deprecated code paths for 2.0.0.
@dgeb to add feature flags for all new public API to this PR.
@dgeb to add a work around (like mentioned above) to ensure that appInstance.container.lookup continues to work.

Mix in to both `Application` and `ApplicationInstance` classes to provide consistent public access to their internally maintained registries.

Pass the related Application, which the instance can inspect to obtain the initialization data that it needs. This eliminates the need to set `applicationRegistry` on the instance, which was always an anti-pattern.

The `RegistryProxy` mixin now maintains the registry instance as `__registry__`. As appropriate, the equivalent `RegistryProxy` methods are called instead of directly accessing the `registry`.

Registry access must go through the Application now.

Now that container will be fully privatized, we can relax naming of members like `registry` that won't be accessed outside of the framework.

Mix in to `ApplicationInstance` class to provide public access to a few select methods on the encapsulated container.

Instead, allow the application's resolver to be invoked only as part of the fallback strategy for the application instance's registry. This has a couple benefits: * It allows registrations made in the app instance's registry to be resolved before falling back to the app's registry, rather than hitting the resolver beforehand. This is important for scenarios such as testing in which custom registrations need to take precedence over the resolver. * It is also more efficient in the case when the app's registry's registrations are used to resolve, because it avoids calling the same resolver twice before finally getting the registration. [Closes emberjs#11174]

* Ensure that `Container#_registry` remains supported for now. Can be deleted once cont/reg reform is enabled by default. * Continue to expose `registry` and `container` on `ApplicationInstance`. These can be removed once cont/reg reform is enabled, but we’ll still need to expose `ApplicationInstance.container.lookup` until 3.0. * Flag deprecation of Application initializer `initialize` arguments.

dgeb · 2015-07-23T04:14:30Z

@rwjblue I've completed the tasks. This has been rebased, feature-flagged, and is ready for review.

Container / Registry Reform

rwjblue · 2015-07-23T05:18:11Z

Thanks @dgeb!

krisselden · 2015-07-23T05:49:45Z

@dgeb if this is supposed to privatize things, did you miss this one? https://github.com/dgeb/ember.js/blob/c353c383b666474e5fb2c2d6fa17934946dcc9ca/packages/container/lib/container.js#L240

should that not be __container__ too?

rwjblue · 2015-07-23T11:55:06Z

@krisselden - This was affecting a public API to be used by initializers and instanceInitializers, but does not change access from within an object looked up from the container. Changing this.container.lookup to this.__container__.lookup would be a pretty big breaking change (and this RFC / PR does not provide an alternative for that use case at all).

I'm totally in favor of exposing some sort of limited public API for use here, but we must tread carefully and ensure no breakage...

dgeb · 2015-07-23T13:39:02Z

@krisselden I personally am in favor of deprecating container in that case too. My preference would be to provide a single path for all lookups - through the app instance. I briefly floated the idea of injecting the app instance as an alternative to container, but response was lukewarm so I didn't pursue in the initial RFC / PR. Now that this PR has been merged, it's probably a good time to reopen this discussion.

Of course, I also agree with @rwjblue that we must tread carefully and ensure no breakage.

dgeb force-pushed the registry-reform branch from 6e9f719 to f5cfa49 Compare June 14, 2015 00:35

dgeb mentioned this pull request Jun 15, 2015

Ember 1.13 orbitjs/ember-orbit#75

Merged

dgeb force-pushed the registry-reform branch from f5cfa49 to e75d440 Compare June 16, 2015 00:37

praxxis mentioned this pull request Jun 19, 2015

Ember 1.13 compatibility (fixes #8) mitchlloyd/ember-islands#9

Merged

chadhietala reviewed Jun 29, 2015
View reviewed changes

dgeb mentioned this pull request Jul 15, 2015

[CLEANUP beta] Remove deprecated registry behaviour #11756

Closed

dgeb force-pushed the registry-reform branch 5 times, most recently from ee45f96 to 81654c0 Compare July 16, 2015 06:11

dgeb force-pushed the registry-reform branch from 81654c0 to 627680c Compare July 16, 2015 17:57

dgeb force-pushed the registry-reform branch 4 times, most recently from 49c95fd to ba47f70 Compare July 21, 2015 15:47

dgeb force-pushed the registry-reform branch 2 times, most recently from a56866c to e180c04 Compare July 22, 2015 13:44

dgeb added 12 commits July 22, 2015 23:49

Introduce RegistryProxy mixin.

ca61792

Mix in to both `Application` and `ApplicationInstance` classes to provide consistent public access to their internally maintained registries.

Comment cleanup / corrections in registry.js.

be5b067

Simplify ApplicationInstance creation.

b06bd88

Pass the related Application, which the instance can inspect to obtain the initialization data that it needs. This eliminates the need to set `applicationRegistry` on the instance, which was always an anti-pattern.

Privatize registry access with dunderscores.

b1cec55

The `RegistryProxy` mixin now maintains the registry instance as `__registry__`. As appropriate, the equivalent `RegistryProxy` methods are called instead of directly accessing the `registry`.

Initializers now just take a single argument: App.

a27dc7a

Registry access must go through the Application now.

Rename Container#_registry to Container#registry.

c125649

Now that container will be fully privatized, we can relax naming of members like `registry` that won't be accessed outside of the framework.

Introduce ContainerProxy mixin.

93a95c6

Mix in to `ApplicationInstance` class to provide public access to a few select methods on the encapsulated container.

Replace double quotes with single quotes.

e64c9ba

Fix app-boot-test to call app.register.

f982e89

Mark ContainerProxy#lookupFactory private.

253edc4

dgeb force-pushed the registry-reform branch from e180c04 to c353c38 Compare July 23, 2015 03:56

rwjblue added a commit that referenced this pull request Jul 23, 2015

Merge pull request #11440 from dgeb/registry-reform

1d7b833

Container / Registry Reform

rwjblue merged commit 1d7b833 into emberjs:master Jul 23, 2015

This was referenced Jul 23, 2015

Registry / Container reform emberjs/rfcs#46

Merged

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

Merged

gpoitch mentioned this pull request Aug 12, 2015

Update ajax initializer to comply with container / registry reform ember-fastboot/ember-cli-fastboot#54

Merged

mike-north mentioned this pull request Aug 16, 2015

Update ember-cli to 1.13.8, use simplified initializer pattern ember-cli/ember-export-application-global#16

Merged

rwjblue mentioned this pull request Aug 16, 2015

2.1 beta blog post emberjs/website#2318

Merged

mike-north mentioned this pull request Aug 23, 2015

Align with container cleanup ember-cli/ember-resolver#109

Merged

abuiles mentioned this pull request Oct 5, 2015

Use public registry. adopted-ember-addons/ember-data-model-fragments#143

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Container / Registry Reform #11440

Container / Registry Reform #11440

dgeb commented Jun 13, 2015

tomdale commented Jun 15, 2015

dgeb commented Jun 18, 2015

dgeb commented Jun 29, 2015

chadhietala Jun 29, 2015

dgeb Jun 29, 2015

dgeb commented Jul 8, 2015

stefanpenner commented Jul 8, 2015

tomdale commented Jul 8, 2015

stefanpenner commented Jul 8, 2015

rwjblue commented Jul 8, 2015

dgeb commented Jul 16, 2015

rwjblue commented Jul 16, 2015

dgeb commented Jul 16, 2015

rwjblue commented Jul 21, 2015

dgeb commented Jul 23, 2015

rwjblue commented Jul 23, 2015

krisselden commented Jul 23, 2015

rwjblue commented Jul 23, 2015

dgeb commented Jul 23, 2015

Container / Registry Reform #11440

Container / Registry Reform #11440

Conversation

dgeb commented Jun 13, 2015

RegistryProxy

ContainerProxy

Privatization of Container and Registry

Simplification of Initializers

Allow Application Instance's Registrations to take Precedence over the Resolver

Remaining Action Items:

tomdale commented Jun 15, 2015

dgeb commented Jun 18, 2015

dgeb commented Jun 29, 2015

chadhietala Jun 29, 2015

Choose a reason for hiding this comment

dgeb Jun 29, 2015

Choose a reason for hiding this comment

dgeb commented Jul 8, 2015

stefanpenner commented Jul 8, 2015

tomdale commented Jul 8, 2015

stefanpenner commented Jul 8, 2015

rwjblue commented Jul 8, 2015

dgeb commented Jul 16, 2015

rwjblue commented Jul 16, 2015

dgeb commented Jul 16, 2015

rwjblue commented Jul 21, 2015

dgeb commented Jul 23, 2015

rwjblue commented Jul 23, 2015

krisselden commented Jul 23, 2015

rwjblue commented Jul 23, 2015

dgeb commented Jul 23, 2015