Skip to content

[FEATURE services] Initial feature #4861

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

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion features.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"ember-runtime-test-friendly-promises": true,
"ember-routing-add-model-option": true,
"ember-routing-linkto-target-attribute": null,
"ember-routing-will-change-hooks": null
"ember-routing-will-change-hooks": null,
"services": null
},
"debugStatements": [
"Ember.warn",
Expand Down
42 changes: 41 additions & 1 deletion packages_es6/ember-application/lib/ext/controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,29 @@ var defaultControllersComputedProperty = computed(function() {
};
});

if (Ember.FEATURES.isEnabled('services')) {
var defaultServicesComputedProperty = computed(function() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

return service;
},
setUnknownProperty: function (key, value) {
throw new Error("You cannot overwrite the value of `services." + key + "` of " + inspect(controller));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}
};
});
}

/**
@class ControllerMixin
@namespace Ember
Expand Down Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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')

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I would typically go with currentLatitude:

var readOnly = Ember.computed.readOnly;

App.NearbyTweetsController = Ember.ArrayController.extend({
  currentLatitude: readOnly('services.geolocation.location.latitude')
});

});
```

@property {Object} services
*/
services: defaultServicesComputedProperty
});

export default ControllerMixin;
59 changes: 59 additions & 0 deletions packages_es6/ember-application/lib/ext/route.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/**
@module ember
@submodule ember-application
*/

import Ember from "ember-metal/core"; // Ember.assert
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doesn't look like this PR uses Ember.assert

import Route from "ember-routing/system/route";

import { get } from "ember-metal/property_get";
import { set } from "ember-metal/property_set";
import { inspect } from "ember-metal/utils";
import { computed } from "ember-metal/computed";

if (Ember.FEATURES.isEnabled('services')) {
var defaultServicesComputedProperty = computed(function() {
var route = this;

return {
container: get(route, 'container'),
unknownProperty: function(serviceName) {
var service = this.container.lookup('service:' + serviceName);

if (!service) {
var errorMessage = "The " + serviceName + " service was accessed from the " + inspect(route) + " route, 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(route));
}
};
});

Route.reopen({
/**
Stores instances of services available for this route to use.
Routes have access to all services defined in the application.
Services provide model information to routes and controllers.

```javascript
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);
};
}
});
```

@property {Object} services
*/
services: defaultServicesComputedProperty
});
}
36 changes: 36 additions & 0 deletions packages_es6/ember-application/tests/system/controller_test.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/*jshint newcap:false */

import { Controller } from "ember-runtime/controllers/controller";
import Service from "ember-runtime/system/service";
import "ember-application/ext/controller";

import Container from "ember-runtime/system/container";
Expand Down Expand Up @@ -153,3 +154,38 @@ test("can unit test controllers with `needs` dependencies by stubbing their `con
equal(broController.get('foo'), 5, "`needs` dependencies can be stubbed");
});

if (Ember.FEATURES.isEnabled('services')) {
test("controllers have access to services", function() {
expect(2);

var container = new Container();

container.register('controller:post', Controller.extend());
container.register('service:geolocation', Service.extend());

var postController = container.lookup('controller:post'),
geolocationService = container.lookup('service:geolocation');

equal(geolocationService, postController.get('services.geolocation'), "registered service is available");

raises(function() {
postController.get('services.nonexistent');
}, ReferenceError, "accessing a non-existent service raises an error");
});

test("can unit test controllers with services dependencies by stubbing their `services` property", function() {
expect(1);

var BrotherController = Controller.extend({
foo: computed.alias('services.sister.foo')
});

var broController = BrotherController.create({
services: {
sister: { foo: 5 }
}
});

equal(broController.get('foo'), 5, "services can be stubbed");
});
}
44 changes: 44 additions & 0 deletions packages_es6/ember-application/tests/system/route_test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
import "ember-application/ext/route";

import Route from "ember-routing/system/route";
import Service from "ember-runtime/system/service";

import Container from "ember-runtime/system/container";

import { computed } from "ember-metal/computed";

if (Ember.FEATURES.isEnabled('services')) {
test("routes have access to services", function() {
expect(2);

var container = new Container();

container.register('route:post', Route.extend());
container.register('service:geolocation', Service.extend());

var postRoute = container.lookup('route:post'),
geolocationService = container.lookup('service:geolocation');

equal(geolocationService, postRoute.get('services.geolocation'), "registered service is available");

raises(function() {
postRoute.get('services.nonexistent');
}, ReferenceError, "accessing a non-existent service raises an error");
});

test("can unit test routes with services dependencies by stubbing their `services` property", function() {
expect(1);

var BrotherRoute = Route.extend({
foo: computed.alias('services.sister.foo')
});

var broRoute = BrotherRoute.create({
services: {
sister: { foo: 5 }
}
});

equal(broRoute.get('foo'), 5, "services can be stubbed");
});
}
6 changes: 6 additions & 0 deletions packages_es6/ember-runtime/lib/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,8 @@ import {
ControllerMixin
} from "ember-runtime/controllers/controller";

import Service from "ember-runtime/system/service";

import RSVP from "ember-runtime/ext/rsvp"; // just for side effect of extending Ember.RSVP
import "ember-runtime/ext/string"; // just for side effect of extending String.prototype
import "ember-runtime/ext/function"; // just for side effect of extending Function.prototype
Expand Down Expand Up @@ -158,6 +160,10 @@ Ember.ObjectController = ObjectController;
Ember.Controller = Controller;
Ember.ControllerMixin = ControllerMixin;

if (Ember.FEATURES.isEnabled('services')) {
Ember.Service = Service;
}

Ember.RSVP = RSVP;
// END EXPORTS

Expand Down
9 changes: 9 additions & 0 deletions packages_es6/ember-runtime/lib/system/service.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import Object from "ember-runtime/system/object";

var Service;

if (Ember.FEATURES.isEnabled('services')) {
Service = Object.extend();
}

export default Service;