-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
[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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe we should add a |
||
} | ||
|
||
return service; | ||
}, | ||
setUnknownProperty: function (key, value) { | ||
throw new Error("You cannot overwrite the value of `services." + key + "` of " + inspect(controller)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
@@ -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') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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') There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, I would typically go with var readOnly = Ember.computed.readOnly;
App.NearbyTweetsController = Ember.ArrayController.extend({
currentLatitude: readOnly('services.geolocation.location.latitude')
}); |
||
}); | ||
``` | ||
|
||
@property {Object} services | ||
*/ | ||
services: defaultServicesComputedProperty | ||
}); | ||
|
||
export default ControllerMixin; |
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
}); | ||
} |
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"); | ||
}); | ||
} |
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; |
There was a problem hiding this comment.
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 thecontrollers
proxy yet Likely to simplify overriding the controllers property for testing?There was a problem hiding this comment.
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.
It should share a common implementation.
There was a problem hiding this comment.
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
forcontrollers
, but removed that for testability.There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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 ofcontroller.controllers.foo.bar
often is only via CP or Binding. So ifcontrollers
is not what it should be the CP and binding value will just benull
orundefined
.Even worse data can flow back up, and set to
controller.controllers.foo
, resulting in unexpected and distracting behavior.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.