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

Support camelcase resolving of controllers in needs array #20

Merged
merged 6 commits into from
Jan 30, 2014
Merged

Support camelcase resolving of controllers in needs array #20

merged 6 commits into from
Jan 30, 2014

Conversation

rahulcs
Copy link
Contributor

@rahulcs rahulcs commented Jan 22, 2014

if a needs array contains a camel case name for a nested controller http://emberjs.com/api/classes/Ember.ControllerMixin.html#property_needs

This program fails on ember-app-kit because the resolver cannot find controller:postComment since it is not handled, which means discrepancy from the ember js syntax for needs.
http://emberjs.jsbin.com/iKAyIjIf/10/edit

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2014

I am unsure of this. I believe that you should be using needs: ['post/comment'] which should work properly with the current implementation. This would then resolve to the following paths (first wins):

app/post/comment/controller.js
app/controllers/post/comment.js

@rahulcs
Copy link
Contributor Author

rahulcs commented Jan 22, 2014

But this does not help someone following the documentation and using EAK and find it not working. Don't you think?

@stefanpenner
Copy link
Contributor

EAK's naming conventions are a strict subset of embers, to prevent ambiguity. If anything we should assert if the usage is malformed.

ember 2.0 will be following these conventions, EAK is pushing everything forward.

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2014

@rahulcs - I agree that it is confusing, but the documentation does not specifically state that it needs to be camelized, and there are likely many examples in the documentation that will need to be fixed before we go live with this as the default resolver in Ember (likely 2.0).

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2014

@stefanpenner - So assert if decamalize(whatever) !== split[1].replace(/\./g, '/')?

@rahulcs
Copy link
Contributor Author

rahulcs commented Jan 22, 2014

Yeah at least an assert would be good if not a documentation change right away.
Let me know if I can push another commit for it. :)

@stefanpenner
Copy link
Contributor

assert with a link to the new conventions
http://iamstef.net/ember-app-kit/guides/naming-conventions.html

@stefanpenner
Copy link
Contributor

i thin kthose are up todate

@rwjblue
Copy link
Member

rwjblue commented Jan 22, 2014

@stefanpenner - OK, will work that up (with some tests).

@rahulcs
Copy link
Contributor Author

rahulcs commented Jan 23, 2014

The build is failing. Will send it again in some time.

@stefanpenner
Copy link
Contributor

can you add a test?

@rahulcs
Copy link
Contributor Author

rahulcs commented Jan 23, 2014

@stefanpenner will add tests and commit.

@@ -117,6 +117,19 @@ test("will raise error if both dasherized and underscored modules exist", functi
}
});

test("will raise error if camelcased modules exist", function() {
expect(2);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will only hit one since the other would require that you NOT hit an error.

stefanpenner added a commit that referenced this pull request Jan 30, 2014
Support camelcase resolving of controllers in `needs` array
@stefanpenner stefanpenner merged commit 37d1c2d into ember-cli:master Jan 30, 2014
@dogawaf
Copy link

dogawaf commented Feb 4, 2014

Hi,
With ember data, I have now assertion thrown when lookup for 'model:staticContent'.
The only workaround that I found is to remove all underscores in my models...

@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2014

@dogawaf - Can you give a bit more information on this in a new issue?

It would be helpful to have examples of your on-disk filenames (just the ones causing an issue), as well as how you are referencing them (if not by auto-lookup). It is quite possible that we need to tweak our assertions, but I'll need examples to add more test coverage.

@dogawaf
Copy link

dogawaf commented Feb 4, 2014

My models directory:

app/models
app/models/static_content.js
app/models/profile.js
app/models/user.js

In my case, the lookup which fails the assert is done automatically by ember-data ActiveModelSerializer, which call store.modelFor('staticContent').
The JSON returned by the server contains the root object static_contents.

@rwjblue
Copy link
Member

rwjblue commented Feb 4, 2014

@dogawaf - #24

abuiles added a commit to abuiles/ember-jj-abrams-resolver that referenced this pull request Feb 19, 2014
This reverts commit 37d1c2d, reversing
changes made to 1d37dc0.

Conflicts:
	dist/ember-resolver-spade.js
	dist/ember-resolver-tests.js
	dist/ember-resolver.min.js
	dist/modules/ember-resolver-tests.js
@abuiles abuiles mentioned this pull request Feb 19, 2014
stefanpenner added a commit that referenced this pull request Feb 19, 2014
kratiahuja pushed a commit to kratiahuja/ember-resolver that referenced this pull request Aug 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants