[FEATURE ember-metal-injected-properties] Injected properties

[slindberg]

Injected Properties

This PR is intended to implement the necessary features for Ember 'services' (see #4861). It introduces 'injection helpers' which are computed properties that are container-aware, but whose use is type dependent (e.g., you cannot define an injected controller property on a view). As discussed on @tomdale's PR, there is ambiguity between an injection helper named Ember.controller and the actual Ember.Controller class, so to avoid this I've created a new top level namespace called inject (see below).

As an example, the following is enabled:

App.PostController = Ember.Controller.extend({
  posts: Ember.inject.controller()
});

Which is functionally equivalent to:

App.PostController = Ember.Controller.extend({
  needs: 'posts',
  posts: Ember.computed.alias('controllers.posts')
});

This PR also adds the controller and service injection helpers (as well as Ember.Service), but those commits can easily be extracted into another PR

Validation

In order to enforce good practices, injection helpers can only be used with certain ember objects (or rather can't be used with certain ember objects).

1. controllers can only be injected into other controllers
2. services cannot be injected into views or components

This validation check can happen in one of two places:

1. at mixin time
2. at instantiation time

The first option seems better since it happens early and only once.

A second type of validation ensures that the object with injection properties has a reference to the container, and that the container has knowledge of the requested injections (through app.register or the resolver). This check can happen at one of two places:

1. at instantiation time
2. at access time

The first option seems better since it happens earlier (assuming that registration of container types happens during app bootstrap).

Injection

When an injection helper property is declared on a class, all instances of that class need the property looked up and set. There are a few places this can happen:

1. at instantiation time via normal container injection
2. at instantiation time in CoreObject
3. lazy lookup on container in computed property getter

The first option would maintain consistency with current container injections and allow instances to access their properties without calling .get(), but require either accessing the injection at mixin time (where a container is not currently accessible), or by altering the container to be aware of the ember object model (which it is currently only vaguely aware of). The last approach has the advantage of not performing a lookup if never accessed, and being able to take advantage of CP cache, which seems to make it the best candidate.

Design Decisions

Feature name

This touches more than just ember-metal, but that package is the lowest common denominator. It would be trivial to split this up into multiple dependent features as an alternative.

inject namespace

This is simply to avoid the potential ambiguity between Ember.controller/Ember.Controller Ember.service/Ember.Service. It also happens to read well, since what the developer wants to do is, for example, 'inject [a] controller'.

inject namespace (not) as a function

Much like Ember.run and Ember.computed, the Ember.inject namespace could potentially be a convenience method for a container injection property, however that would enable the "injection of anything into anything" anti-pattern that should be avoided if possible. By only allowing injected properties through predefined methods, Ember is enforcing best practices.

Referencing the container in ember-metal

The container is not mentioned anywhere in ember-metal, but since injected properties are type of descriptor, and all other descriptor properties are defined in ember-metal, it seemed like the best place. It could also easily go in the ember-runtime package.

Validation in Ember.Object init event (no longer applies)

There are a few places instantiation-time validation can happen:

1. directly in CoreObject's constructor
2. Object's init` event/hook
3. Object's didDefineProperty hook

Although nothing in Ember core (to my knowledge) currently uses the init event, it seemed like the best option since requiring all existing subclasses that define init or didDefineProperty hooks call _super for validation to work, is IMHO dangerous and could be considered a breaking API change. Yes, controllers currently implement this check in the init hook, but extending it to all objects is much farther reaching.

willMergeMixin on Ember.Object

This is obviously dubious for the potential performance hit. However, during my ember-metal spelunking, it seems like this private hook is specifically intended for doing any special mixin-time handling. So short of baking injected property validation into ember-metal itself, this is all I could come up with.

Branding vs. instanceof

This is something that @stefanpenner mentioned in the original PR. I could not find much information on the performance implications of "branding" versus use of the instanceof operator, except by inference in this jsperf (which only shows results in chrome). This did not seem to warrant breaking existing patterns, and can easily be changed later when other instanceof checks are converted.

Ember.assert vs. throw new EmberError(...)

To be perfectly honest, I have no idea when something warrants a full production-time exception over a development assertion. Is there a guide for this anywhere? I ended up just making best-guesses.

TODO

It should be possible to cache a list of injected properties on the constructor's meta object when mixin-time validation happens, thereby avoiding the repeated descriptor lookups at instantiation-time validation. I just don't know enough about how metadata is stored/accessed to be confident enough to do this, especially since there are additional considerations when classes get extended multiple times with new injected properties.

[rwjblue]

Would this work instead?

ComputedProperty.prototype.teardown.call(this, obj, keyName);

My thoughts were basically, just to ensure the same things that were setup by calling the ComputedProperty constructor are torn down in the future. Currently, what this has is perfectly fine, but to stay in sync I think maybe we should use the upstream teardown...

[rwjblue]

Looks pretty good on first review. Will dig a little deeper and answer some of your questions in the intro later tonight or tomorrow.

[stefanpenner]

this code already exists in the container, can we instead have it scope up these requirements, and validate there?

[stefanpenner]

additionally, for internals we do not use on('init') instead we place the method explicitly in the constructor to ensure ordering

[stefanpenner]

as I think about this, this validations do not occur soon enough, as they occur after the users init hook, leaving these property injections potentially invalid for constructor use.

I believe the only way to fix this is to address my above concern

[slindberg]

I think that only leaves modifying the constructor of CoreObject, correct? Also, what exactly did you mean by "scope up these requirements" (let the container throw the exception when the lookup fails?)

[stefanpenner]

yes exactly. Note this will likely require changes to the container itself.

this also addresses: "Referencing the container in ember-metal" as injections and validations would only occur if the entity in question was instantiated by the container.

[stefanpenner]

also, good job on this PR. 🍻

[stefanpenner]

you usage of throw new EmberError(...) is correct, asserts are only meant as nice to haves during development, but to be stripped out in production. Your usage is for rules that must be strictly abided by in production aswell

[slindberg]

Awesome, thanks! I addressed the super constructor and Ember.create bits, and I'll look more into pushing validation into the container tonight.

[stefanpenner]

@slindberg awesome, this is great work! Keep me posted, i'll provide feedback as time permits.

[tomdale]

Nice job. I need to review in closer detail but this is very promising.

[slindberg]

@stefanpenner Looking at the container module it looks like you were referring to the logic in buildInjections, which does a full lookup of all keys and throws if one is not resolved. I see a few paths forward:

1. Simply export the logic in the on('init') handler and call it in CoreObject's constructor (this would not address your concerns about duplicating logic in the container).
2. Either push buildInjections into the container API or expose something similar like validateInjections (which wouldn't do a full lookup), and call it from CoreObject's constructor. This still would require exporting some logic to gather injected properties and convert to the correct format.
3. Go all out and build into the container the ability to to gather injections via introspection (injectionsFor?), i.e. with a specially named factory method, and have the container do all of the work of validating and injecting. Note that this would make InjectedProperty descriptors obsolete and also bring injected properties more in line with how they currently work, e.g. this.store.

The current implementation tries to stay hands-off the container API and therefore away from number the third option (see the "Injection" section in the PR comment), mostly because creating a way for objects to communicate with the container seemed like a bad idea. Maybe it's not though, and establishing some kind of 'needs' dependency specification API for all objects in lieu of constructor parameters isn't a bad thing. Note: I'm pretty sure the current register API can't/shouldn't be used for this, but I could be missing something.

Short of going the third route, I don't see how this could be accomplished:

this also addresses: "Referencing the container in ember-metal" as injections and validations would only occur if the entity in question was instantiated by the container.

Thoughts? (Thanks for the quick feedback already!)

[stefanpenner]

this._super$Constructor $ makes it clear that this is an internal non-colliding thing. In theory this replicates what es6 super would do

[slindberg]

I looked through the codebase for that pattern and didn't find anything. I guess I just didn't want to be the first one to do it 😛

[stefanpenner]

Either push buildInjections into the container API or expose something similar like validateInjections (which wouldn't do a full lookup), and call it from CoreObject's constructor. This still would require exporting some logic to gather injected properties and convert to the correct format.

👍 validateInjections within the container, which would extract a list of fullName dependencies from the factory for AOT validatation, and then the descriptor allows for lazy lookup (via get)

(3.) is partially aligned and partially not. We only want to use the container for validation at time of object instantiation, but the actual lookups should happen lazily. This gives us the nice fail-fast validation of constructor injections, but while enabling lazy lookup and class level annotation.

[stefanpenner]

Simply export the logic in the on('init') handler and call it in CoreObject's constructor (this would not address your concerns about duplicating logic in the container).

I may be wrong, but i really feel this validation is not CoreObject's concern at all, but only when an object is instantiated via the container should it be a concern. This enables very simple unit testing

for example:

export default Ember.Controller.extend({
  twitter: Ember.inject.service()
})

in unit tests can very simply just instantiate by providing collaborators at construction:

var controller = Controller.create({
  twitter: twitterCollaborator
})

In normal app usage, via injection (or lookup) the container does what the container is good at, managing dependencies and as such the validations for lazy dependencies.

[slindberg]

@stefanpenner I refactored the instantiation-time validation to happen in the container. Modified the container slightly by adding a validateInjections method (which isn't behind the feature flag), and added a check for a new lazyInjections method on factories which is supposed to return an array of full names. Not sure about the name; it's also currently defined in CoreObject's ClassMixin since calling reopenClass on Ember.Object immediately after creating it seemed strange.

Still feels a little weird for the container to be aware of injections and not do the injecting itself. Let me know what you think.

[mixonic]

@slindberg This is great, thanks! I'm a little concerned about having the validation mixin applied to all objects- is that overkill? Does it make sense to scope that mixin onto routes, controllers, and services for now? Would like your thoughts on it.

@stefanpenner Can you say more about why you want lazy lookup here? I see it helping to avoid an up-front performance hit from looking up the dependency chain of an object eagerly, but I'm not sure that would often be a real world challenge. I'm happy with it being lazy, but would like to know if there is a specific justification.

[stefanpenner]

@mixonic

1. absorbs cycles
2. only instantiates on demand (perf scenario)

[slindberg]

@mixonic In order to be able to enforce that, for instance, a controller cannot be injected into a model, the validation step has to be performed at mixin time of the model, which means the model class must have the InjectedPropertyValidation mixin somewhere in its inheritance hierarchy. Unfortunately I think this means that the validation must happen on all Ember.Object descendants. On the plus side, it only has to happen once for each class -- but I still have the same concerns.

Currently the validation functions only perform assertions, and so they are no-ops in production except that all supporting overhead would still be there. One solution is to just convert the asserts into real run-time errors, which I'm conflicted about. Another is to potentially defeaturify more than just the asserts, and do away with the validation mixin in production entirely, which I don't think is currently possible (note that this would also make _super calls in willMergeMixin problematic).

@mixonic, @stefanpenner In regards to the perf case, I'm beginning to think that there's not much to be gained from lazy injections given the current uses (controllers and services). Controllers in the current route chain are nearly always already fully looked up by the router before the controllers get a chance to use them, so delaying lookup doesn't provide much benefit. And since the nature of services is that they are singletons and small in number, if they exist in the app they are likely to be looked up during its lifecycle at least once anyway, so eager lookup is fine (although perhaps not within a highly complex and segmented app). I am also assuming that there isn't a terrible amount of overhead for factory injections in the container itself, which there doesn't appear to be. Thoughts?

[stefanpenner]

@slindberg the cycle case is very common, its not just 1 step removed cycles but in larger apps cross dependencies (although maybe not ideal) across many steps are very common and easily solvable by laziness.

Additionally the cost of instantiating a large number of dependencies that wont be used till after boot is also concerning.

I am bullish on laziness for this feature.

[mixonic]

I also favor the laziness. Pondered it last night, I just wanted to see Stef's arguments to back up my own.

@slindberg gotcha re: the object-level mixin. It is definitely super unfortunate. Trying to think of something nicer, but I see what you are talking about.

[slindberg]

@stefanpenner makes sense. Re a real-world example of these cycles: an app that has a dense controller dependency graph, so eager injection would end up instantiating a large number of potentially unneeded controllers at app boot. If I understand correctly, this definitely should be avoided. It should also be possible to mark these injections as eager on a case-by-case basis for people who want to tune their app performance where it makes sense.

Did you get a chance to look at my container validation changes?

[stefanpenner]

@slindberg im interested in "eager" vs "lazy" but i suspect we can get away with always lazy.

[stefanpenner]

@slindberg I'm going to make time this evening to thoroughly review the current implementation :)

[slindberg]

@stefanpenner thanks 😃

I'm on no schedule for this feature, I'm mostly just excited to be able to contribute to Ember!

[stefanpenner]

is it possible to ensure this transform isn't needed at this point? Can injections already be parsed and normalized?

[slindberg]

Def a little weird. Updated to change the lazyInjections hook to return a hash that is normalized and then passed to validateInjections.

[mmun]

@slindberg This is great! 🍻 Regarding "Referencing the container in ember-metal"... It's fine to define the descriptor in ember-runtime instead. I agree that this descriptor is really about working with containers, which isn't inherently a property of ember-metal.

[mmun]

If injections are intended to be used on a DS.Model, something similar to 05143e6 may need to be added depending on how soon this is merged.

[tomdale]

@slindberg Do you think you'll have an opportunity to update this PR to address @stefanpenner's concerns above? I would LOVE to get this shipped in the next release.

[slindberg]

@tomdale I'm happy to create a new PR, but @stefanpenner's concerns were never fully clear to me (otherwise I would have tried to address them before this was merged). Let's make sure we're all on the same page.

To clarify the current state of injected property validation, there are two types of validation that occur:

1. Type validation - this ensures that type A can be injected into type B based on arbitrary rules. The only type validation that currently exists prevents controllers from being injected into non-controllers. This happens in the willMergeMixin hook of Ember.Object, so is evaluated only once for each class at app boot time. I consider this strictly a debugging aid, and so is executed inside an Ember.assert, meaning that in production builds the only overhead is an empty function call per class.
2. Container validation - this ensures that the injected object A actually exists in the container when instantiating object B via the container. This happens in the container's instantiate method by calling the lazyInjections hook on the factory to gather all container keys that can potentially be looked up by accessing injected properties, and throwing if they don't exist in the container. This is not a debugging aid, as missing objects will likely lead to ReferenceErrors, and so generates a full-fledged Error (like other lookups of non-existent objects currently do).

Reading over @stefanpenner's concerns:

Eager validation and lazy instantiation is key, validation should happen at the container when constructor injections are validated.

• lazyInstantion to allow for cycles and reduce upfront object instantiation cost
• validations to mitigate painful trolling

and

it should happen at instantiation time, or at factory lookup time just like other injection validations

It sounds like (2.) container validation as implemented does not need to be changed, and that (1.) type validation should happen in the container's factoryFor. This would entail adding some kind of postLookup factory hook that does what willMergeMixin currently does. I would argue that since this is a debugging aid only, it doesn't warrant extending the container's API with another hook. However, if @stefanpenner disagrees, or sees another path to validation at factory lookup time, I'll be happy to open a new PR.

The other option is of course to get rid of type validation altogether, which may make sense given that controllers are going to be replaced with routeable components in the near future. Please let me know how to proceed on this.

[stefanpenner]

The PR is close, but my concerns are as follows:

Currently it is possible to satisfy the lazyInjection between the time that the factory is created and the time it's first instance instantiates. I don't believe this window is needed and if used will very likely by brittle. I would like to close this window.

Additionally the current implementation re validates lazyInjections on each object instantiation. This is wasteful as the fate should already be known at the time the factory is created.

My recommendation, although I am open to other suggestions, would be to validate the lazy injections when the factory is first created and but before it is cached. Likely here and here

It is possible I am overlooking some detail that makes this not possible, but I believe it is sound and will yield the best possible solution. If problems do exist let me know and I will gladly provide more suitable advice.

[stefanpenner]

if hash is always Object.create(null) we can mitigate the slow hasOwnProperty check here.

[slindberg]

To me, this is a question of the degree to which the container should be considered a separate microlib. AFAICT it currently maintains reasonably loose coupling with the rest of Ember, so I hesitate to remove this check even though its safety could be internally guaranteed. If I had to make the call, I would keep the check in at the slight perf cost. Do you disagree?

This is also related to whether the lazyInjections method on ember objects should be considered public or not.

[slindberg]

@stefanpenner thanks for all the feedback, I'll open a new PR to address your concerns. However, you haven't weighed in on what I called 'type validations'. How important is it to enforce that controllers are not injected into non-controllers?

[stefanpenner]

@slindberg that part of the request is a @tomdale + @wycats high level concern they might be best to respond to that not me.

I do really like the onLookup('controller:foo', function(entity) { } hook you briefly mentioned as it would be awesome for other things. like preventing people from eagerly constructing or instantiating in initializers

[stefanpenner]

Also, after reviewing this again, I wonder (and I hope we can, but maybe we can't) close that window between factory creation and first instantiation.

This is what I know for sure:

• at the very least we must only invoke lazyInjections once per class, as it is going to be extremely slow. (So worst case we cache, and maybe bust on reopens...)

This is what I am unsure about at this moment:

• the lazyInjections code may depend on more reopen's happening so closing the window will prevent some absolutely legitimate forms of extension, for example:

export default {
  name: 'thing',
  initialize(container, application) {
    container.lookupFactory('something:else').reopen({
      foo: Ember.inject(somethingValid)
    });
  }
};

Now it is technically possible to reopen after the first instantiation, but this is honestly the root of many ember performance issues, gnarly bugs, and anxiety induced receding hairlines and as such I would love to no longer cater to it.

[tomdale]

@stefanpenner Do people really reopen factories they've retrieved from the container? I'd think you'd just reopen the base class.

[slindberg]

I think no matter how classes are reopened, there's always the potential for container lazy lookup validation to become stale (e.g. one initializer runs and looks up something:else and runs validations, then the next one reopens the ElseSomething class by module reference and adds an injected property).

After using my co-worker as a rubber duck for a solid 15 minutes, I can think of two sane paths forward:

1. Perform lazy lookup validations at factory lookup time and deal with reopening classes by exposing some callback mechanism for the container to re-run validations (e.g. .on('reopen')).
2. Officially deprecate reopen after create is called, and perform lazy lookup validation at instantiation time backed by a (yet another) cache to prevent multiple lazyInjections calls.

The second option allows classes to be reopened after container lookup and avoids making the container aware of even more model internals, which I'm personally in favor of. However deprecating reopen after instantiation is a big change to the Ember object model, and I don't know if it's acceptable to allow undefined behavior to occur even when a deprecation warning happens (e.g. a class is reopened after objects are instantiated, a deprecation warning is logged, and then a seemingly unrelated Attempting to inject an unknown injection... error blows up the app).

I'll be working on updates on this branch, and when we've figured out how I should address the above issues I'll open a new PR.

[slindberg]

@stefanpenner @tomdale ping

[stefanpenner]

@slindberg

Perform lazy lookup validations at factory lookup time and deal with reopening classes by exposing some callback mechanism for the container to re-run validations (e.g. .on('reopen')).
Officially deprecate reopen after create is called, and perform lazy lookup validation at instantiation time backed by a (yet another) cache to prevent multiple lazyInjections calls.

It seems like the window we want to enable should leave open factoryLookup -> create, but post create we don't care anymore.

So the validation should occur (but be cached) on first instantiation. Logging or asserting seems fine, i believe we can support this case but I REALLY WANT IT TO DIE... Reopens post first instance creation costs us much pain and I will be advocating we kill it in the future.

[slindberg]

@stefanpenner Sounds good, I'll get to work on caching the lazy lookup validation on first instantiation.

For anyone curious, I was surprised to find out that reopening a factory registered with the container seems to have no effect on the factory or any instances of it created before or after reopening: jsbin. This is rather convenient for the purposes of validating injected properties, since it's already not possible to reopen after instantiation.

[slindberg]

Concerns addressed in #9760

[sandstrom]

@slindberg This is an awesome feature! Thank you! 😄

During a refactor I've moved many things from my home-made services structure into Ember Services. However, for some use-cases I'd like to use something different, and it would be neat to create custom injections.

So, my question, do you know why createInjectionHelper is not available as a public API? Was there any discussion around this?

I can see the downside of people using it incorrectly, but that can be handled using documentation. Ember does surface quite a bit of useful low-level functionality in other areas (e.g. around the run-loop). Would be neat if createInjectionHelper was public too!

[stefanpenner]

So, my question, do you know why createInjectionHelper is not available as a public API? Was there any discussion around this?

The concern boiled down to allowing arbitrary instance <-> instance injections. This confuses the data-flow model.

We feel good about allowing service -> component direct communication (since the life-cycles enable it) but not very good about aribtraryThingWithItsOwnLifeCycle < otherArbitraryThingWithItsOwnLifeCycle.

I think the easiest way to illustrate the limitation, is to align it slightly with the bindings down actions up strategy.

[sandstrom]

Got it. I'll respect whatever you prefer — i.e. it's a bad idea to make a change to Ember if I'm the only one with a need for it :) But count this as one token in the make-it-public bucket.

I've got one use-case where this would be useful, and still wouldn't collide with the limitation above (but it's narrow, not generalizable). And I think the limitation could also be handled by documenting clearly the intended usage.

I'll find a work-around for my case (but if many others start asking for this it may be worth revisiting this).

---

Upgrading our app to 1.13 and a more component-driven structure. Works splendid so far! You [the core team] has done an amazing job! ⛵

[stefanpenner]

Share our usecase :) the limitation is really conservative today.

[tim-evans]

I would like to create an injection helper for a pattern called 'actions', which are functions that do some business logic:

For example, in actions/attach-image-to-comment.js

export default function (comment, file) {
  let store = this.get(comment, 'store');
  return file.upload().then(function (data) {
    let attachment = store.createRecord('attachment', {
      comment: comment,
      url: data.headers.Location
    });
    return attachment.save();
  });
}

This nicely encapsulates somewhat complicated behavior into a function. In our route, I might just care about testing that the action got called. In that case, it would be nice to swap out the action for a mock that returned a promise that I had control over in the test. That way, I could say that:

1. Yes, the action was called
2. Subsequent behaviors are called

This also removes complications around testing these usually tricky bits of business logic that cause a lot of friction to test.

[tomdale]

@tim-evans I have had this issue as well. I've wanted to inject a function, but found myself having to create a service with a single method on it—and having to do a little song and dance just to expose one method felt very Java-like to me (in the bad way).

I for one would definitely be open to an RFC explaining the primitive and making it possible for folks to inject functions.

[rwjblue]

FWIW: it is possible today with something like:

export default {
create: function() {
return someSpecialFunctionYouImported;
}
}

[rwjblue]

But I absolutely agree that we should make it more ergonomic (and think through the ramifications) via an RFC.

[tim-evans]

@tomdale @rwjblue Followed your advice and created an RFC