Skip to content
This repository has been archived by the owner. It is now read-only.

Instance initializers deprecation #1951

Merged
merged 2 commits into from Aug 11, 2015

Conversation

@tomdale
Copy link
Member

commented Jan 22, 2015

Adds a deprecation guide for the deprecations introduced by instance initializers. Should be merged with emberjs/ember.js#10256.

name: "clock",
initialize: function(instance) {
var clock = instance.container.lookup("clock:main");

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

doing a lookup in a container will likely break things, especially when introducing 3rd party add-ons. Can we not advertise this footgun approach? We likely need to add some construct to solve the pain of removing this, but it is important we do.

We have discussed this at at least 2 F2F meetings, documenting it seems like a poor idea.

This comment has been minimized.

Copy link
@tomdale

tomdale Jan 22, 2015

Author Member

What is the footgun you are concerned about? A brief search of GitHub (https://github.com/search?utf8=%E2%9C%93&q=initializer+container.lookup&type=Code&ref=searchresults) shows many, many uses of container.lookup() in initializers.

We need to give people a transition path right now (this is a deprecation guide, not regular documentation). Do you think we should hold off on incremental changes until a solution that eliminates container.lookup in initializers entirely is in place?

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

What is the footgun you are concerned about?

A brief search of GitHub (https://github.com/search?utf8=%E2%9C%93&q=initializer+container.lookup&type=Code&ref=searchresults) shows many,

that search is extremely scary, for example each occurrence of lookup('store:main') (their are many) in that search fails to specific its initializer ordering dependency as after ember-data, which means it is entirely luck, that their application boots. Any change to the initializer dep graph can result in a topology change that may result in a broken app.

This isn't just hypothetical, i have witnessed this in the wild an unfortunate amount of times.

So, this means eager instantiating during initializers destabilizes application boot.

An example: 2 independent initializers that both appear reasonable, do not work together.

Please note, the most problematic cases are when these initializers exist in add-ons, but the issue still remains without add-ons.

// initializer1.js
container.inject('router:main', 'km', 'service:kiss-metrics')
container.lookup('router:main').on('didtransition', function() { this.km.track(..) });

// initializer2.js
container.inject('router:main', 'google', 'service:google-analtyics')
container.lookup('router:main').on('didtransition', function() { this.google.track(..) });

Depending on the initialization order, either google analytics or kiss metrics will be broken. Why, because the the eager lookup, proceeds the injection rule of the later initializer.

proposed solution.

// initializer1.js
container.inject('router:main', 'km', 'service:kiss-metrics')
container.onLookup('router:main', function(router) {
  router.on('didTransition', function() { this.km.track(..) })
});

// initializer2.js
container.inject('router:main', 'google', 'service:google-analtyics')
container.onLookup('router:main', function(router) {
  router.on('didTransition', function() { this.google.track(..) })
});

This comment has been minimized.

Copy link
@stefanpenner

stefanpenner Jan 22, 2015

Member

We need to give people a transition path right now (this is a deprecation guide, not regular documentation). Do you think we should hold off on incremental changes until a solution that eliminates container.lookup in initializers entirely is in place?

Guiding people down a path that is fundamentally broken and uses private API seems irresponsible. The above mentioned API should be a reasonably quick addition and then this deprecation entry can merely suggest the correct path forward.

@wycats

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

@stefanpenner Thanks so much for clarifying the specific issue you are worried about.

This exact kind of problem was, indeed, a motivator for the API that I initially designed with Dan in December, talked about with the core team, and have finally implemented here.

To be concrete: the problem is that it is important for all app-wide injection rules to be set up before any instances are created through the container.

In the new design, which this deprecation guide is helping people transition to, App.initializers ("boot initializers") only have access to a registry, which they can use to set up injections. App.instanceInitializers have access to the container, which they can use to instantiate objects. All boot initializers are guaranteed to run before the instance initializers.

Indeed, the deprecation warning that caused a user to find this web page is catching the precise bug you are worried about. That is because the registry passed into boot-time initializers has deprecated access to lookup. That means that all of the broken examples you are worried about will begin to see deprecation warnings that move the lookup after all injections have already completed.

Can you see any ways in which the new system would still exhibit the bug you are worried about.

@stefanpenner

This comment has been minimized.

Copy link
Member

commented Jan 22, 2015

@wycats ah I see, my mistake the instanceInitializer is post initialization :shipit: this is great!!!

curious, why not have instance.lookup rather then instance.container.lookup

#### Deprecate Access to Instances in Initializers

Previously, initializers had access to an object that allowed them to
both registry new classes and get instances of those classes.

This comment has been minimized.

Copy link
@jgwhite

jgwhite Jan 22, 2015

Contributor

s/registry/register

@wycats

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

@stefanpenner I was worried that people would refactor initializers that look like this:

App.initializer({
  name: 'foo',
  initialize: function(container) {
    container.lookup('foo:bar');
  }
});

into:

App.instanceInitializer({
  name: 'foo',
  initialize: function(container) {
    container.lookup('foo:bar');
  }
});

But container is not actually a container here! This pattern would likely get cargo culted and propagate confusion.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2015

@jgwhite Fixed, thanks!

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2015

@wycats I disagree with you a little bit here.

I think the case you're highlighting is valid, but in practice I believe it sacrifices long-term ergonomics over hypothetical, short-term confusion. We have made changes in the past where the objects passed to hooks have changed, but their most popular methods remained to help in migration, and it has not caused mass bedlam yet.

My feeling is that, instead of passing the registry/container, and the potential for access to all sorts of private things, we instead pass the Application/ApplicationInstance, and add methods that proxy to the public methods of the registry and container, respectively.

@wycats

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

@tomdale I disagree with you here, but I think we can let it simmer.

@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 23, 2015

How me and @wycats have technical discussions.
website 2015-01-22 16-41-10

@dgeb

This comment has been minimized.

Copy link
Member

commented Jan 23, 2015

Here is my attempt to thread this needle:

I consider the Registry and Container interfaces to be a level below what should be exposed to developers.

I agree with @tomdale that we can transition to passing the Application to initializers (or "appInitializers") and the ApplicationInstance to instanceInitializers (or "appInstanceInitializers") without migration problems.

We already expose the registry's register and inject methods on the Application. These proxies essentially form the public API to a Registry, and should be sufficient for initializers.

Similarly, we can also expose the container's lookup method on the instance. However, I share @wycats' concern about inevitable cargo-culting and propose that we deprecate ApplicationInstance#lookup in favor of a method (perhaps resolve?) that internally proxies to __container__.lookup. In this way we can raise warnings when people access ApplicationInstance#lookup without losing the benefits of limiting the surface area of Container that gets exposed. (Note: I'm not dead-set on the name resolve, esp. because it is similar but different from Registry#resolve, but I think the general approach is sound.)

Here are some more benefits to the proxy approach:

  • Proxies could short-circuit registrations/injections after a particular point in the application lifecycle. Once the first instance has been built, Application#register and Application#inject could fail.
  • Container and Registry could remain private. However, if passed to initializers, they really should be public and fully documented.
  • An instance-specific registry that falls back to using the application's registry could be useful for A/B testing, in which different instances should have different injections/registrations. The ApplicationInstance could expose register and inject methods that would operate only on the instance-specific registry. Perhaps this instance-specific registry wouldn't even be available by default (and would have to be specifically enabled) to discourage its use except when truly needed?
@tomdale

This comment has been minimized.

Copy link
Member Author

commented Jan 26, 2015

@dgeb Thank you Dan, that is a thorough articulation of what I'd like to see happen.

mixonic added a commit to mixonic/website that referenced this pull request Mar 23, 2015
@rwjblue

This comment has been minimized.

Copy link
Member

commented May 28, 2015

Needs a rebase...

@ghost

This comment has been minimized.

Copy link

commented Jun 11, 2015

The docs for the initializer deprecation http://emberjs.com/deprecations/v1.x/#toc_deprecate-access-to-instances-in-initializers could really use an ES6 module exports example in the code snippet. The initializer blueprint doesn't produce code that looks like that, so it's a little tricky to figure out (read: I don't get it).

Here's one that came up in the latest Ember Weekend podcast that is covered by this deprecation, I think: https://gist.github.com/code0100fun/f9b99b2a562702683602#file-app_initializers_session-js

@rwjblue

This comment has been minimized.

Copy link
Member

commented Jun 13, 2015

@tomdale - Needs a rebase...

@wycats wycats force-pushed the app-instances branch from 21975dd to e33c152 Aug 11, 2015
wycats added a commit that referenced this pull request Aug 11, 2015
Instance initializers deprecation
@wycats wycats merged commit 4e0562e into master Aug 11, 2015
@wifelette wifelette deleted the app-instances branch Oct 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
7 participants
You can’t perform that action at this time.