[FEATURE services] Initial feature #4861

Closed
wants to merge 1 commit into
from

Projects

None yet
Owner
tomdale commented May 13, 2014

This is an initial spike of the described in this thread on the discussion forums.

The TL;DR is that both controllers and routes get a services hash that they can look up services on; it's very similar to the controllers hash on controllers and, indeed, is implemented very similarly.

To create a geolocation service, for example, just define an Ember.Service class like so:

App.GeolocationService = Ember.Service.extend({
  getPosition: function() {
    return new Ember.RSVP.Promise(function(res, rej) {
      if (!geolocation) {
        rej(new Error("Geolocation is not available either in this browser or on this device."));
        return;
      }

      geolocation.watchPosition(function(position) {
        res(position);
      });
    });
  }
});

If you're using ember-cli, just export an Ember.Service from app/services/geolocation.js.

Once that's done, you have access to your service in both routes and controllers:

     App.NearbyTweetsRoute = Ember.Route.extend({
       model: function() {
         var geolocation = this.get('services.geolocation'),
             twitter = this.get('services.twitter');

         return geolocation.currentLocation().then(function(lat, lng) {
           return twitter.findNearbyTweets(lat, lng);
         };
       }
     });

(Note that this deviates slightly from my original post on the forums; in order to avoid naming collisions, everything is namespaced under the services hash instead of being injected directly onto the route/controller.)

Tasks left to do before this should be shipped to master:

  • If possible, share implementation of services hash generation across controllers/routes
  • If possible, share more implementation between services hash and controllers hash
  • Write more inline documentation and guides to explain how services fit in conceptually

I'm a little embarrassed at how small the actual implementation is. It feels like it's doing almost nothing, which is basically true. But I hope this can be a starting point for the community to start evolving on our shared understanding of what services are.

Owner

With this approach we lose the declarative upfront validation step needs. Which is a big bummer. I suppose with SA we can validate...

Another concern with using "services" as our controllers are proxies to the underlying data models, this addition is a breaking change. I know we don't like it, but maybe we should prefix these things with something like angular's $, or ideally ☃

@stefanpenner stefanpenner commented on the diff May 13, 2014
packages_es6/ember-application/lib/ext/controller.js
@@ -60,6 +60,29 @@ var defaultControllersComputedProperty = computed(function() {
};
});
+if (Ember.FEATURES.isEnabled('services')) {
+ var defaultServicesComputedProperty = computed(function() {
stefanpenner
stefanpenner May 13, 2014 Owner

I wonder if the whole property should also be marked as readOnly We don't do that with the controllers proxy yet Likely to simplify overriding the controllers property for testing?

stefanpenner
stefanpenner May 13, 2014 Owner

this form of CP proxy, is now duplicated 3 times.

  1. controller.controllers
  2. controller.services
  3. route.services

It should share a common implementation.

rwjblue
rwjblue May 13, 2014 Owner

@stefanpenner - You are correct, we used to have this readOnly for controllers, but removed that for testability.

stefanpenner
stefanpenner May 13, 2014 Owner

I would really like if we could re-enable when not testing. This is a common self troll for people.

rwjblue
rwjblue May 13, 2014 Owner

Also, there is a specific test below that asserts that you can simply specify the services property for testing.

tomdale
tomdale May 14, 2014 Owner

One option would be to offer a testing helper for stubbing services. Something like this:

injectServices(controller, {
  geolocation: myGeolocationStub
});

It could use defineProperty to override the read-only CP. That being said, I'd really hate to lose the obviousness of just replacing the property and creating yet-another concept that people have to understand in order to be able to test properly.

@stefanpenner Can you maybe expand on what the self-troll case is? This isn't something I've ever run into so I want to understand why it's been so painful for you.

stefanpenner
stefanpenner May 14, 2014 Owner

In practice people do silly things, or during refactoring accidents happen. This seems to happen more the larger the code base, and the larger the teams, especially if they are inexperienced. Additionally, when this does happen it often manifests as an intermittent failure and is very tricky to track down.

controller.controllers should never be set directly, if it is set, remember that consumption of controller.controllers.foo.bar often is only via CP or Binding. So if controllers is not what it should be the CP and binding value will just be null or undefined.

Even worse data can flow back up, and set to controller.controllers.foo, resulting in unexpected and distracting behavior.

var obj = Ember.Object.create();
Ember.set(obj, 'controllers.foo.bar', 2)
// =>  Property set failed: object in path "foo" could not be found or was destroyed.

I also assure your that when I originally made this property readOnly it was after encountering the issue in the wild multiple times. I was very surprised, and each occurrence was a very unpleasant debugging experience.

Ideally controllers property should not be directly set-able, but overridable at construction time.

@stefanpenner stefanpenner commented on the diff May 13, 2014
packages_es6/ember-application/lib/ext/controller.js
+ var controller = this;
+
+ return {
+ container: get(controller, 'container'),
+ unknownProperty: function(serviceName) {
+ var service = this.container.lookup('service:' + serviceName);
+
+ if (!service) {
+ var errorMessage = "The " + serviceName + " service was accessed from the " + inspect(controller) + " controller, but no service with that name was found.";
+ throw new ReferenceError(errorMessage);
+ }
+
+ return service;
+ },
+ setUnknownProperty: function (key, value) {
+ throw new Error("You cannot overwrite the value of `services." + key + "` of " + inspect(controller));
stefanpenner
stefanpenner May 13, 2014 Owner

Ember.Error ? (needs controllers proxy likely also wants this)

@stefanpenner stefanpenner commented on the diff May 13, 2014
packages_es6/ember-application/lib/ext/controller.js
@@ -60,6 +60,29 @@ var defaultControllersComputedProperty = computed(function() {
};
});
+if (Ember.FEATURES.isEnabled('services')) {
+ var defaultServicesComputedProperty = computed(function() {
+ var controller = this;
+
+ return {
+ container: get(controller, 'container'),
+ unknownProperty: function(serviceName) {
+ var service = this.container.lookup('service:' + serviceName);
+
+ if (!service) {
+ var errorMessage = "The " + serviceName + " service was accessed from the " + inspect(controller) + " controller, but no service with that name was found.";
+ throw new ReferenceError(errorMessage);
stefanpenner
stefanpenner May 13, 2014 Owner

maybe we should add a Ember.ReferenceError that inherits from Ember.Error

@stefanpenner stefanpenner commented on the diff May 13, 2014
packages_es6/ember-application/lib/ext/controller.js
@@ -166,7 +189,24 @@ ControllerMixin.reopen({
@property {Object} controllers
@default null
*/
- controllers: defaultControllersComputedProperty
+ controllers: defaultControllersComputedProperty,
+
+ /**
+ Stores instances of services available for this controller to use.
+ Controllers have access to all services defined in the application.
+ Services provide model information to routes and controllers.
+
+ ```javascript
+ App.NearbyTweetsController = Ember.ArrayController.extend({
+ currentLatitude: function() {
+ return this.get('services.geolocation.location.latitude');
+ }.property('services.geolocation.location')
stefanpenner
stefanpenner May 13, 2014 Owner

also expressed as

currentLatitude: Ember.computed.readOnly('services.geolocation.location.latitude')
currentLatitude: Ember.computed.oneWay('services.geolocation.location.latitude')
currentLatitude: Ember.computed.alias('services.geolocation.location.latitude')
rwjblue
rwjblue May 13, 2014 Owner

Agreed, I would typically go with currentLatitude:

var readOnly = Ember.computed.readOnly;

App.NearbyTweetsController = Ember.ArrayController.extend({
  currentLatitude: readOnly('services.geolocation.location.latitude')
});
@stefanpenner stefanpenner commented on the diff May 13, 2014
packages_es6/ember-application/lib/ext/route.js
@@ -0,0 +1,59 @@
+/**
+@module ember
+@submodule ember-application
+*/
+
+import Ember from "ember-metal/core"; // Ember.assert
stefanpenner
stefanpenner May 13, 2014 Owner

doesn't look like this PR uses Ember.assert

Owner

another thing to consider is that other users could already be using service:<name>.
I would also also reclaim config: but to do so we need to first deprecate and reclaim.

Although the container isn't public api app.inject and app.register are, this then enables users to have purposed service already.

Owner

should we consider moving data to a service?

Owner

Everyone is always on my case about semver but services on an ObjectController is a breaking change. It is a common word likely to be used as a model association.

Owner
rwjblue commented May 16, 2014

I agree with @krisselden about taking away the top level term services. I have at least 2 applications (possibly more) that have a Service model (think where services are rendered in a professional setting).

That being said, we just need to decide on a prefix to use that doesn't indicate private (like an _ would), but also doesn't remove a reserved word from application domains. There are many other scenarios where a policy on this would be useful.

I know that @stefanpenner votes for ☃, but I am unsure we can get a concensus on that one...

jonnii commented May 16, 2014

maybe namespace it like app/services?

Owner
tomdale commented May 19, 2014

After discussing this at the core team meeting, we all have concerns about reserving a services property on ObjectControllers post-1.0.

One thing that we noted is that nearly EVERY controller we've looked at has the following pattern:

App.PostController = Ember.ObjectController.extend({
  needs: ['application'],
  application: Ember.computed.alias('controllers.application')
});

The same thing would probably happen with the services API as implemented in its current form.

This is obviously extremely redundant, and if there's anything I hate, it's boilerplate. What if we could have a mechanism for specifying both the "needs" and the property to alias to, all in one?

App.PostController = Ember.ObjectController.extend({
  application: Ember.controller('application')
});

Services derive quite nicely from this as well:

App.PostController = Ember.ObjectController.extend({
  geolocation: Ember.service('geolocation')
});

This approach let's us enforce layering (since we will raise an error if you try to use, e.g., Ember.service CPs inside a component) and also allows us to raise early if we detect that you've requested a controller or service that wasn't found in the container.

Owner
rwjblue commented May 19, 2014

@tomdale - This also provides the benefit of allowing the developer to choose the alias to use therefore avoiding the name collision concerns mentioned above.

Member

What is our justification for Ember.service('foo') and Ember.controller('foo') over Ember.lookup('service:foo') and Ember.lookup('controller:foo')?

Owner
tomdale commented May 19, 2014

What is our justification for Ember.service('foo') and Ember.controller('foo') over Ember.lookup('service:foo') and `Ember.lookup('controller:foo')?

@lukemelia, the justification is:

  1. We don't want to expose container keys as first-class public APIs.
  2. We want to enforce that certain types are only injected in certain other types; lookup is too generic.

I really like the idea of having Ember.container and Ember.service and I'm glad there's a push to come to a consensus about best practices around using services. Conversations like these are one of the reasons I appreciate using Ember.

That said, @lukemelia's comment resonates with me. I think one of the problems with Ember is it's MASSIVE public api surface area. Choosing Ember.service('foo') over Ember.lookup('service:foo') further increases the API surface area for what seems to be relatively little gain.

In addition to creating another abstraction to learn, it also buts up against the 0, 1, or many rule. If controllers and services are so similar, it seems Ember.controller and Ember.service could potentially lead to another similar concepts (e.g. Ember.bus, etc)

We don't want to expose container keys as first-class public APIs.

I'd love to hear more about your reasons for this. From what I've been seeing in blogs and conference talks, most devs who are writing and speaking about the container are already talking about container keys as being first class public APIs. Personally, I'm a big fan -- container keys are a a simple and well designed way of organizing dependencies and I've enjoyed using the api in my app.

We want to enforce that certain types are only injected in certain other types; lookup is too generic.

This comment makes me think you've been seeing DI used in bizarre and unsustainable ways. What's your experience been? How are devs misusing it? If abuse is rampant, "enforcement" is probably wise. However, if this is a attempt to prevent abuse, it feels premature to me.

Owner

@scottmessinger it's not that DI is itself unsustainable, but injecting anything on anything without thought will lead to much pain. If one wishes todo this one can use the existing container app.inject API directly (in an initializer).

@tomdale would like the sugar he proposes to guide best practices.

Owner
machty commented May 20, 2014

@tomdale why not also support

App.PostController = Ember.ObjectController.extend({
  geolocation: Ember.service()
});

where the service is inferred by the property name unless you provide a string arg to target another service.

Owner
rwjblue commented May 20, 2014

@machty - I like that. Could make some pretty nice syntax with a stashed var:

//app/controllers/post.js

var service = Ember.service;

export default Ember.ObjectController.extend({
  zomgAwesome: service()
});
Member

Is the testing API not public? One argument for exposing container keys as part of the API is that it is necessary knowledge for testing (and also makes testing more intuitive).

ahx commented May 21, 2014
App.PostController = Ember.ObjectController.extend({
  application: Ember.controller('application')
});

@tomdale This would replace the needs the needs syntax, right? So this would become the recommended way to manage dependencies between controllers as described in the guides, right?
http://emberjs.com/guides/controllers/dependencies-between-controllers/

Owner

@ahx yes

Owner
ebryn commented May 21, 2014

I'm concerned with the ambiguity of Ember.Controller and Ember.controller

Owner
ebryn commented May 21, 2014

I'm also interested in the implementation details...

Owner

@ebryn ya that ambiguity isn't great

Owner
machty commented May 21, 2014

@ebryn @stefanpenner what do you think of the idea of a more general concept whereby ember-metal will collect properties that are instanceof some special macro type and expose them so that libraries like ED, EM, EPF, et al don't need to manually loop over class definitions to detect these special properties themselves? Ember.service() and Ember.controller() would also return a value of this type and they'd be collected by ember-metal at mixin time so that the injection logic that happens later can just loop over these already-collected properties rather than having to traverse at that time.

Owner

@machty the idea is good. I would be concerned about more instanceof checks, we should instead consider branding these objects... Speaking of this we should brand Alias instead of the constant instanceof checks...

Owner
ebryn commented May 21, 2014

We already have this basically, it's called didDefineProperty

On Wed, May 21, 2014 at 1:53 PM, Stefan Penner notifications@github.comwrote:

@machty https://github.com/machty the idea is good. I would be
concerned about more instanceof checks, we should instead consider branding
these objects... Speaking of this we should brand Alias instead of the
constant instanceof checks...


Reply to this email directly or view it on GitHubhttps://github.com/emberjs/ember.js/pull/4861#issuecomment-43791551
.

Owner
tomdale commented May 23, 2014

@ebryn @stefanpenner I think the ambiguity goes away with ES6 modules.

@Panman8201 Panman8201 referenced this pull request in ember-cli/ember-cli Jun 19, 2014
Closed

Web workers support? #1011

Contributor

Super excited about the prospect of a formalized service type in Ember. I already have several 'services' that fit the mold perfectly (authentication manager, analytics tracking, etc.). Also happy about the decision to make consumers specify their own dependencies instead of the service being aware of consumers. The potential to remove all of the needs: [ ... ]/...: Ember.computed.alias('controllers...') boilerplate is just a bonus 😄.


@tomdale I'm confused about how modules would resolve the Controller/controller ambiguity. Is it because at some point ember properties that are classes will be imported like this:

import { Controller } from 'ember';

export default Controller.extend({ ... });

And that injection helpers would never (or couldn't) be imported in the same way? Another solution would be to namespace them under Ember.inject, since there's already precedent for that kind of scoping, e.g. Ember.computed.

export default Ember.Controller.extend({
  zomg: Ember.inject.service(),
  awesome: Ember.inject.controller('awesomeThings'),
  ...
});

@machty forgive my ignorance, but why would libraries like ED, etc. need to do anything with these injections? It seems to me like these helpers map to container types, which those libraries aren't concerned with. Do you anticipate giving libs the ability to create custom injection helpers?

export default Ember.Controller.extend({
  store: DS.store('main'),
  ...
});

If so, wouldn't that defeat the point of these helpers by enabling "injecting anything on anything without thought" as @stefanpenner said? Or perhaps the process of creating a custom injection helper qualifies as 'thought'? 😉

Owner
rwjblue commented Jul 13, 2014

@tomdale - Updates? Is this in progress?

Contributor

@tomdale, @stefanpenner, @rjackson: just dropped a PR that takes a stab at this #5162.

Note to self: don't reference issue numbers from commits when rewriting history.

Owner

@slindberg nice PR, its a good start :)

Owner
machty commented Jul 14, 2014

Yeah, for serious.

Owner
tomdale commented Aug 8, 2014

Closing in favor of #5162.

@tomdale tomdale closed this Aug 8, 2014
@marcoow marcoow referenced this pull request in simplabs/ember-simple-auth Jan 25, 2017
Merged

add missing needs #1181

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