Skip to content
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

needs naming is incosistent with container lookup #2025

Closed
darthdeus opened this issue Feb 9, 2013 · 15 comments
Closed

needs naming is incosistent with container lookup #2025

darthdeus opened this issue Feb 9, 2013 · 15 comments
Assignees
Labels
Milestone

Comments

@darthdeus
Copy link
Member

As it was pointed out in #2000, when specifying a controller name for the container lookup one should use posts.index if it is the controller which belongs to a posts.index route. This works out great when using App.__container__.lookup("controllers:posts.index"), but when specifying needs

Here's a JSBin illustrating the issue

App.FooController = Ember.Controller.extend({
  needs: ["posts.index", "postsIndex"]
});

results in controllers.posts.index being undefined, while postsIndex returns a new instance of the App.PostsIndexController which isn't the one associated with the posts.index route.

This means that the App.FooController doesn't isn't able to reach into the content of posts.index controller.

@oldfartdeveloper
Copy link
Contributor

I just created an issue that is very similar. My problem is that there aren't any diagnostics on this; the binding just fails and nothing tells you why.

Thanks for posting this

@rickard2
Copy link

Thanks, I can confirm this is an issue for me as well.

@heycarsten
Copy link

I've been bitten by this one too.

@ashenden
Copy link

Bitten. Halp.

@ghost ghost assigned wycats Feb 14, 2013
@wagenet
Copy link
Member

wagenet commented Feb 14, 2013

This should be an easy one to fix. But the question is how we should be normalizing it.

@wagenet
Copy link
Member

wagenet commented Feb 15, 2013

I think that whatever we do here, we can probably avoid breaking existing behavior so, while we need to fix this, we can probably wait until after RC.

@darthdeus
Copy link
Member Author

Since the only actual problem is that you can't use route name for the lookup, we could easily resolve this by supporting it. As you pointed out in #2000 it is not a bug that postsIndex behaves differently than posts.index.

The only problem I see here is that if we have needs: "posts.index" and then controllers.posts.index, it is not certain if we want index property on PostsController, or PostsIndexController. Even if we made the later always take precedence, it would cause the un-solvable scenarios (PostsController with detail property and PostsDetailController, which both are controllers.posts.detail).

We could say that in this case you can't simply have a detail property on the parent if the child controller has the same name. If we chose to go down the path where postsIndex and posts.index is the same thing, it would solve the issue, but it also feels like a big inconsistency with how the container works.

@endash
Copy link
Contributor

endash commented Feb 15, 2013

How about converting "posts.index" to "posts_index" and otherwise having it behave identically. I would--naively--expect that if needs: ["postsIndex"] results in a PostsIndexController instance as a property of controllers, then needs: ["posts.index"] would also have the PostsIndexController from the container as a property on controllers as well, rather than a nested structure.

@darthdeus
Copy link
Member Author

That would solve the issue, but then it's not consistent with how the container works, unless we change that as well.

@alexspeller
Copy link
Contributor

This seems to be a big problem - there is no way to use needs to access a controller that is not single-syllably named atm? Does anyone have a workaround for using needs to access something like FooIndexController?

@darthdeus
Copy link
Member Author

@alexspeller afaik the only workaround is to use the deprecated this.controllerFor("foo.index")

@alexspeller
Copy link
Contributor

Great, thanks!

@wycats
Copy link
Member

wycats commented Feb 20, 2013

The solution is to have a normalize hook in the container that gets called before resolution and lookup. Ember.Application would implement the normalize hook.

@inkredabull
Copy link

Was bitten by the bug causing this but pulled and built b22afef and can now make progress.

One thing, for a controller like BarIndexController as in:

http://jsfiddle.net/Fs4DJ/1/

... is there a reason:

this.get('controllers.bar.new')

...should be undefined? I would expect 'bar.new' to resolve to the controller defined for the route.

@tonywok
Copy link

tonywok commented Mar 22, 2013

I'm running into the same issue as @inkredabull. For example, I'm not sure where to tell the container to register controller:bars.index to be App.BarsIndexController.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests