Automatically synthesize the controller 'needs' dependencies #1731

Merged
merged 1 commit into from Jan 16, 2013

Conversation

Projects
None yet
@klaaspieter
Contributor

klaaspieter commented Jan 8, 2013

Add a little bit of hidden magic that automatically synthesizes properties for dependencies specified using needs (issue #1713). The following:

App.PostController = Em.Controller.extend({})
App.CommentsController = Em.Controller.extend({
  needs: 'post'
})

would result in a controller that verifies that the post controller exists and exposes a postController property defined as:

postController: function() {
    this.controllerFor('post');
}.property();

I feel this would make the needs property work as expected. Without the pull request needs would only verify that the dependency exists in the container with no side effects if the controller does exist.

I don't consider this pull request complete but wanted to get it out there so we can have a public discussion about needs in general. I also feel that my current implementation isn't the best way of doing it so feedback on that is welcome too :)

@klaaspieter

View changes

packages/ember-routing/lib/ext/controller.js
@@ -8,7 +8,7 @@ Ember.ControllerMixin.reopen({
this._super.apply(this, arguments);
// Structure asserts to still do verification but not string concat in production
- if(!verifyDependencies(this)) {
+ if (!synthesizeDependencies(this)) {

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 8, 2013

Contributor

I'm not a fan of synthesizeDependencies being responsible for both verifying that the dependency exists in the container and synthesizing the property. I left it this way because I feel there needs to be a discussion about whether we want needs to synthesize or not and this was the fasest way to get it working.

If we decide that needs should synthesize I'm open for suggestions on how to make this better :)

@klaaspieter

klaaspieter Jan 8, 2013

Contributor

I'm not a fan of synthesizeDependencies being responsible for both verifying that the dependency exists in the container and synthesizing the property. I left it this way because I feel there needs to be a discussion about whether we want needs to synthesize or not and this was the fasest way to get it working.

If we decide that needs should synthesize I'm open for suggestions on how to make this better :)

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 8, 2013

Contributor

In addition to both verifying and synthesizing I feel there must be a better location to do this then init. Again suggestions are welcome.

@klaaspieter

klaaspieter Jan 8, 2013

Contributor

In addition to both verifying and synthesizing I feel there must be a better location to do this then init. Again suggestions are welcome.

@klaaspieter

View changes

packages/ember-routing/tests/system/controller_test.js
@@ -19,6 +19,21 @@ test("If a controller specifies a dependency, it is available to `controllerFor`
equal(postsController, postController.get('postsController'), "Getting dependency controllers work");
});
+test ("If a controller specifies a dependancy, the property is automatically synthesized", function() {

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 8, 2013

Contributor

This test is basically a duplicate of the dependency test. They can be rolled into one but I thought it better for the sake of the pull request to have a test that explicitly tests the behavior that was added.

@klaaspieter

klaaspieter Jan 8, 2013

Contributor

This test is basically a duplicate of the dependency test. They can be rolled into one but I thought it better for the sake of the pull request to have a test that explicitly tests the behavior that was added.

@klaaspieter

View changes

packages/ember-routing/lib/ext/controller.js
@@ -33,20 +33,28 @@ Ember.ControllerMixin.reopen({
}).property('content')
});
-function verifyDependencies(controller) {
+function synthesizeNeedsProperty(controller, dependency) {
+ Ember.defineProperty(controller, dependency + "Controller", Ember.computed(function() {

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 11, 2013

Contributor
  1. There must be a better way to turn the dependency into a capitalized property name
  2. Is needs supposed to accept other dependencies besides controllers? If so, this will break 👎
@klaaspieter

klaaspieter Jan 11, 2013

Contributor
  1. There must be a better way to turn the dependency into a capitalized property name
  2. Is needs supposed to accept other dependencies besides controllers? If so, this will break 👎
@leepfrog

This comment has been minimized.

Show comment Hide comment
@leepfrog

leepfrog Jan 15, 2013

I would ❤️ this because it would reduce a handful of lines in each controller that has a side-dependency...

👍 :shipit:

I would ❤️ this because it would reduce a handful of lines in each controller that has a side-dependency...

👍 :shipit:

@colymba

This comment has been minimized.

Show comment Hide comment
@colymba

colymba Jan 15, 2013

👍 makes sense to me.. if I define a dependency it is very likely that I will access that controller... why not make it readily accessible...

colymba commented Jan 15, 2013

👍 makes sense to me.. if I define a dependency it is very likely that I will access that controller... why not make it readily accessible...

@josepjaume

This comment has been minimized.

Show comment Hide comment
@josepjaume

josepjaume Jan 15, 2013

Contributor

👍

Contributor

josepjaume commented Jan 15, 2013

👍

@sohara

This comment has been minimized.

Show comment Hide comment
@sohara

sohara Jan 15, 2013

👍 I'm adding boilerplate init handlers in my controllers to access the controllers as properties. Would be nice if the needs API took care of that.

sohara commented Jan 15, 2013

👍 I'm adding boilerplate init handlers in my controllers to access the controllers as properties. Would be nice if the needs API took care of that.

@tchak

This comment has been minimized.

Show comment Hide comment
@tchak

tchak Jan 15, 2013

Member

I think we should start with this #1774
And try to find a way to bind to foreign controller properties without exposing properties.

Member

tchak commented Jan 15, 2013

I think we should start with this #1774
And try to find a way to bind to foreign controller properties without exposing properties.

@klaaspieter

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 15, 2013

Contributor

@tchak the annoying thing about the way #1774 does it is that you have to manually create a property for every dependency you want to expose because you can't bind to controllerFor. Having a needs property is already being explicit about your dependencies, I don't think you should also have to manually create that to make it even more explicit.

Contributor

klaaspieter commented Jan 15, 2013

@tchak the annoying thing about the way #1774 does it is that you have to manually create a property for every dependency you want to expose because you can't bind to controllerFor. Having a needs property is already being explicit about your dependencies, I don't think you should also have to manually create that to make it even more explicit.

@wycats

This comment has been minimized.

Show comment Hide comment
@wycats

wycats Jan 16, 2013

Owner

@klaaspieter Having thought about this more, I'd prefer if the API was:

controller.get('controllers.controllerName')

The way this should work is that we have a controllers object that implements unknownProperty to delegate to the controllers, checking needs first. This will allow other properties to easily depend on these controllers.

Think you can swing it? I'll pull it quickly 😉

Owner

wycats commented Jan 16, 2013

@klaaspieter Having thought about this more, I'd prefer if the API was:

controller.get('controllers.controllerName')

The way this should work is that we have a controllers object that implements unknownProperty to delegate to the controllers, checking needs first. This will allow other properties to easily depend on these controllers.

Think you can swing it? I'll pull it quickly 😉

@klaaspieter

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 16, 2013

Contributor

Shouldn't be to hard to change, @wycats you sure you don't want to do it yourself? 😉

Contributor

klaaspieter commented Jan 16, 2013

Shouldn't be to hard to change, @wycats you sure you don't want to do it yourself? 😉

@leepfrog

This comment has been minimized.

Show comment Hide comment
@leepfrog

leepfrog Jan 16, 2013

Isn't this how it previously was? lol.. i wish you had made this decision before refactoring all of our code.. ;)

Isn't this how it previously was? lol.. i wish you had made this decision before refactoring all of our code.. ;)

@klaaspieter

This comment has been minimized.

Show comment Hide comment
@klaaspieter

klaaspieter Jan 16, 2013

Owner

The additional check wouldn't be needed if #1729 was merged as well. This for block could be changed to just return controller.controllerFor(controllerName).

The additional check wouldn't be needed if #1729 was merged as well. This for block could be changed to just return controller.controllerFor(controllerName).

wycats added a commit that referenced this pull request Jan 16, 2013

Merge pull request #1731 from klaaspieter/synthesize-needs
Automatically synthesize the controller 'needs' dependencies

@wycats wycats merged commit 892390c into emberjs:master Jan 16, 2013

@tshi0912

This comment has been minimized.

Show comment Hide comment
@tshi0912

tshi0912 May 23, 2013

{needs: 'post'} does not work if the target controller is generated manually, see:

App.PostController = Em.Controller.extend({})
App.CommentsController = Em.Controller.extend({
needs: 'post'
})

var commentsController = App.CommentsController.create();

Result:
TypeError: container is null
[在此错误处中断]

return container.lookup('controller:' + controllerName);

{needs: 'post'} does not work if the target controller is generated manually, see:

App.PostController = Em.Controller.extend({})
App.CommentsController = Em.Controller.extend({
needs: 'post'
})

var commentsController = App.CommentsController.create();

Result:
TypeError: container is null
[在此错误处中断]

return container.lookup('controller:' + controllerName);

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner May 23, 2013

Owner

@tshi0912 you must instantiate the controller from the parents container, in most contexts where you need to do this there should be a this.controllerFor helper. This is needed, in-order to benefit from the entire dependency injection system.

This aspect currently is missing a guide, that being said some api docs do exist:
https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L375-L421
https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/ext/controller.js#L48-L72
https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/route.js#L387-L406

Owner

stefanpenner commented May 23, 2013

@tshi0912 you must instantiate the controller from the parents container, in most contexts where you need to do this there should be a this.controllerFor helper. This is needed, in-order to benefit from the entire dependency injection system.

This aspect currently is missing a guide, that being said some api docs do exist:
https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/system/application.js#L375-L421
https://github.com/emberjs/ember.js/blob/master/packages/ember-application/lib/ext/controller.js#L48-L72
https://github.com/emberjs/ember.js/blob/master/packages/ember-routing/lib/system/route.js#L387-L406

@tshi0912

This comment has been minimized.

Show comment Hide comment
@tshi0912

tshi0912 Jul 11, 2013

@stefanpenner How should I create controller in its parents container, could give me some hint?

@stefanpenner How should I create controller in its parents container, could give me some hint?

@klaaspieter klaaspieter deleted the klaaspieter:synthesize-needs branch Jul 11, 2013

@goodcode

This comment has been minimized.

Show comment Hide comment
@goodcode

goodcode Sep 12, 2013

I have the same issue as @tshi0912. I want to create unit tests for those controllers, but I can't even instantiate them.

I have the same issue as @tshi0912. I want to create unit tests for those controllers, but I can't even instantiate them.

@goodcode

This comment has been minimized.

Show comment Hide comment
@goodcode

goodcode Sep 12, 2013

@tshi0912 OK, I managed to achieve my goal, using similar approach to the approach used in this test:

test("If a controller specifies a dependency, it is accessible", function() {
var container = new Ember.Container();
container.register('controller', 'post', Ember.Controller.extend({
needs: 'posts'
}));
container.register('controller', 'posts', Ember.Controller.extend());
var postController = container.lookup('controller:post'),
postsController = container.lookup('controller:posts');
equal(postsController, postController.get('controllers.posts'), "controller.posts must be auto synthesized");

@tshi0912 OK, I managed to achieve my goal, using similar approach to the approach used in this test:

test("If a controller specifies a dependency, it is accessible", function() {
var container = new Ember.Container();
container.register('controller', 'post', Ember.Controller.extend({
needs: 'posts'
}));
container.register('controller', 'posts', Ember.Controller.extend());
var postController = container.lookup('controller:post'),
postsController = container.lookup('controller:posts');
equal(postsController, postController.get('controllers.posts'), "controller.posts must be auto synthesized");

@stefanpenner

This comment has been minimized.

Show comment Hide comment
@stefanpenner

stefanpenner Sep 12, 2013

Owner

@goodcode seems good. I have been spiking on some solutions to make this easier. Expect to see it soon in github.com/stefanpenner/ember-app-kit

Owner

stefanpenner commented Sep 12, 2013

@goodcode seems good. I have been spiking on some solutions to make this easier. Expect to see it soon in github.com/stefanpenner/ember-app-kit

@sly7-7

This comment has been minimized.

Show comment Hide comment
@sly7-7

sly7-7 Sep 12, 2013

Contributor

@stefanpenner @goodcode In unit test, I think I would simply be able to create the controller, passing its dependencies in the constructor. Something like

App.MyController = Ember.Controller.extend({
    needs: 'posts'
})

MyControllerUnderTest = App.MyController.create({
    posts: PerhapsSomeMockedDependency.create()
})

Am I totally crazy ? I find it weird to be obliged to create a container, register the thing, and look them up for unit tests.

Contributor

sly7-7 commented Sep 12, 2013

@stefanpenner @goodcode In unit test, I think I would simply be able to create the controller, passing its dependencies in the constructor. Something like

App.MyController = Ember.Controller.extend({
    needs: 'posts'
})

MyControllerUnderTest = App.MyController.create({
    posts: PerhapsSomeMockedDependency.create()
})

Am I totally crazy ? I find it weird to be obliged to create a container, register the thing, and look them up for unit tests.

@wycats

This comment has been minimized.

Show comment Hide comment
@wycats

wycats Sep 12, 2013

Owner

You could do something like this:

App.MyController = Ember.Controller.extend({
    needs: 'posts'
});

MyControllerUnderTest = App.MyController.create({
  controllers: {
    posts: PerhapsSomeMockedDependency.create()
  }
})

Remember that needs just creates the controllers hash for you, so you should be able to replace it with whatever you want.

Owner

wycats commented Sep 12, 2013

You could do something like this:

App.MyController = Ember.Controller.extend({
    needs: 'posts'
});

MyControllerUnderTest = App.MyController.create({
  controllers: {
    posts: PerhapsSomeMockedDependency.create()
  }
})

Remember that needs just creates the controllers hash for you, so you should be able to replace it with whatever you want.

@sly7-7

This comment has been minimized.

Show comment Hide comment
@sly7-7

sly7-7 Sep 12, 2013

Contributor

@wycats Oh, yes, that's great then, Perhaps the test mentioned by @goodcode should be refactored using this way. If so, this is one small thing I'm capable of.

Contributor

sly7-7 commented Sep 12, 2013

@wycats Oh, yes, that's great then, Perhaps the test mentioned by @goodcode should be refactored using this way. If so, this is one small thing I'm capable of.

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