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

The render helper is not looking up controllers properly #2723

Closed
seanrucker opened this issue May 23, 2013 · 20 comments
Closed

The render helper is not looking up controllers properly #2723

seanrucker opened this issue May 23, 2013 · 20 comments
Assignees

Comments

@seanrucker
Copy link

In the blog post announcing RC2 it specifically states with regard to the render helper that:

If you want to use an alternative namespace, you can use a /-separated path.

{{render 'blog/posts'}}

This will render the blog/posts template with the controller Blog.PostsController

This is incorrect because the render helper is still using the old dot notation. Specifically this line is responsible for causing the lookup to fail:

https://github.com/emberjs/ember.js/blob/v1.0.0-rc.3/packages/ember-routing/lib/helpers/render.js#L43

Commenting out fixes the problem.

@twokul
Copy link
Member

twokul commented May 29, 2013

Seems to work with RC-4. Or am I missing something here?

http://jsbin.com/axixuf/2

@jonnii
Copy link

jonnii commented May 29, 2013

Your controller isn't in a namespace. I think this is actually still an issue. I think something like this should work, but I'm not exactly sure how you create a new namespace...

http://jsfiddle.net/8mNjc/1/

@seanrucker
Copy link
Author

This is still an issue from what I can tell. You're jsbin shows that it correctly pulls the template, but it doesn't run with the proper controller. This shows what I'm talking about:

http://jsbin.com/upoxuc/1/

@jonnii
Copy link

jonnii commented May 29, 2013

If you change your controller name to App.BlogIndexController then it'll work. The question is, is this supposed to resolve the controller in the namespace, App.BlogIndexController or either?

@seanrucker
Copy link
Author

According to the Ember blog it should lookup Blog.IndexController.

"This will render the blog/posts template with the controller Blog.PostsController."

http://emberjs.com/blog/2013/03/30/ember-1-0-rc2.html

@ghost ghost assigned wagenet May 29, 2013
@krisselden
Copy link
Contributor

@wagenet I'm assigning you simply because you are the author of 0ebaef1 feel free to reassign

@wagenet
Copy link
Member

wagenet commented May 29, 2013

I'm pretty sure my commit was added before namespaces. We decided to treat slashes the same as dot notation for ease of use. With namespaces, this is no longer the behavior we want. The only risk to removing this is if people were relying on the slash notation for the dot notation behavior. Maybe someone could put an error handler in so that if the namespace doesn't exist then it tells people that they might want to try dot notation?

@balinterdi
Copy link
Contributor

@wagenet The error handler you mention would basically just inform the developer that slashes can't be used to delimit namespaces, and that she should use dots to achieve this, right?

@balinterdi
Copy link
Contributor

It seems to me that neither {{ render 'blog.index' }} (see http://jsfiddle.net/LNWwH/5) nor {{ render 'blog/index' }} (see http://jsfiddle.net/balinterdi/LNWwH/4/) looks up the namespaced controller and thus neither of these versions work.

@zeevallin
Copy link

This is the only way I could get it to work.

template:

{{render "sidebar/user" model.user controller='app/Sidebar/User'}}

controller:

App.Sidebar ||= Ember.Namespace.create()
App.Sidebar.UserController = Ember.Controller.extend()

seanrucker pushed a commit to seanrucker/ember.js that referenced this issue Jul 29, 2013
@seanrucker
Copy link
Author

I put through a PR for this issue: #3063

@wagenet There is already an existing error message. If you try to use slash notation and the namespace doesn't exist it displays:

"Assertion failed: You are looking for a index view in the Blog namespace, but the namespace could not be found"

@wagenet
Copy link
Member

wagenet commented Jul 29, 2013

Just a heads up, @stefanpenner decided not to merge @seanrucker's PR. We're doing some more in-depth overhauling of the container normalization which should cover this.

@stefanpenner
Copy link
Member

we want to encapsulate the normalization rules to the resolver. So it will becomes easy to switch out as peoples module/resolution needs change. A side effect, is that we should be able to provide a defaultResolver with todays broken behaviour, for those that don't care. While allowing people to opt into a more modern resolver.

@ghost ghost assigned stefanpenner Aug 24, 2013
@stefanpenner
Copy link
Member

An update, the resolver now encapsulates the normalization rules: b7fe725

This will allow us to continue to support the existing behaviour, but allow others to opt-in to a more consistent experience, like we do in EAK: https://github.com/stefanpenner/ember-app-kit/blob/master/vendor/loader.js#L41

As the slash clearly causes issues, we will likely see a different mechanism of declaring a namespace. The current thought is @

<type>:<name>@<namespace>
view:button@bootstrap

or more related to the above issue, maybe something like:

{{render 'posts@blog'}}

@stefanpenner
Copy link
Member

it looks like we will add a flag, so people can toggle this behaviour. as '/' for namespace doesn't work anymore. Then we can transition people to a above described pattern

@wagenet
Copy link
Member

wagenet commented Apr 12, 2014

@stefanpenner, does this even make sense anymore?

@stefanpenner
Copy link
Member

So I'm not terribly motivated to hack a solution as in the new module world everything works wonderfully.

At some point we should sort this out, but I don't have the time to fix this. If someone has time, I will give them advice

@wagenet
Copy link
Member

wagenet commented Jul 28, 2014

Closing for now, but PRs are welcome.

@wagenet wagenet closed this as completed Jul 28, 2014
@stefanpenner stefanpenner reopened this Jul 28, 2014
@stefanpenner
Copy link
Member

please aggregate this into "deprecate slash as namespace"

@wagenet
Copy link
Member

wagenet commented Jul 28, 2014

See #5260

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

No branches or pull requests

8 participants