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

Pod routable component ignored if next to route.js in same directory #12154

Closed
mdehoog opened this issue Aug 20, 2015 · 4 comments
Closed

Pod routable component ignored if next to route.js in same directory #12154

mdehoog opened this issue Aug 20, 2015 · 4 comments
Assignees

Comments

@mdehoog
Copy link

@mdehoog mdehoog commented Aug 20, 2015

I'm using both pods and routable components (thanks to PR #11939) in my application. I would love for the route.js definition for each routable component to sit next to the component itself, just like you can do with controllers. For example, I structure my routable component home as follows:

app
    pods
        components
            home
                component.js
                route.js
                template.hbs

I then map the route:

this.route('components/home', {path: '/'});

In route.js I render the home component using renderTemplate:

export default Ember.Route.extend({
  renderTemplate: function () {
    this.render({component: 'home'});
  }
});

However, the component object is never actually created, as the conditional (

if (!template && !ViewClass && Component && isGlimmerComponent) {
) is not entered because template is defined. This is because the route and the template have the same path. The template is rendered, but without using component.js.

I would have thought that, if component is defined in the options passed to the render method AND that component exists and satisfies the requirements (is a glimmer component), it should prefer the component and not fallback to the template.

I suggest that !template is removed from the conditional statement, or at least ignored if component is defined in the options and the component matches routable component requirements.

@rwjblue
Copy link
Member

@rwjblue rwjblue commented Aug 20, 2015

I am unsure about the reasoning behind that conditional, assigning to @ebryn for review.

mdehoog added a commit to mdehoog/ember.js that referenced this issue Aug 22, 2015
(Fixes emberjs#12154)

If there's a template the same name as the route, that template is used instead of a routable component even if a component name is passed in the options hash to `route.render`.
@mdehoog
Copy link
Author

@mdehoog mdehoog commented Aug 24, 2015

In case it's useful, here's a jsbin demonstrating the issue: http://emberjs.jsbin.com/gipicibobo/1/edit?html,js,output

Here is the same jsbin, but using an Ember build with the !template conditional removed: http://emberjs.jsbin.com/tabebemilu/1/edit?html,js,output.

Branch here: https://github.com/mdehoog/ember.js/tree/pod-routable-components-fix, happy to raise a PR.

mdehoog added a commit to mdehoog/ember.js that referenced this issue Sep 1, 2015
(Fixes emberjs#12154)

If there's a template the same name as the route, that template is used instead of a routable component even if a component name is passed in the options hash to `route.render`.
@mdehoog
Copy link
Author

@mdehoog mdehoog commented Sep 1, 2015

If anyone else runs into this issue, I'm currently extending the ember-resolver to resolve routes sitting next to renderable components as follows:

App = Ember.Application.extend({
  Resolver: Resolver.extend({
    resolveRoute: function (parsedName) {
      const componentsFullName = parsedName.type + ':components/' + parsedName.fullNameWithoutType;
      return this._super(parsedName) || this._super(this.parseName(componentsFullName));
    }
  })
});

This has the added benefit of not requiring the components/ prefix for routable-component routes defined in router.js and in templates.

This issue can be closed as the above works well for now, and the routable components implementation is likely to change as the RFC (emberjs/rfcs#38) is implemented.

@rwjblue
Copy link
Member

@rwjblue rwjblue commented Sep 1, 2015

Thanks for following up and providing a solution!

@rwjblue rwjblue closed this Sep 1, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.