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

Add failing test for @content-helper lookup #105

Merged
merged 4 commits into from
Aug 6, 2015
Merged

Add failing test for @content-helper lookup #105

merged 4 commits into from
Aug 6, 2015

Conversation

tim-evans
Copy link
Contributor

Related to #101

@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2015

Ok. Now fix it ;)

@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2015

Looks good to me.

@stefanpenner - Thoughts?

@stefanpenner
Copy link
Contributor

can someone describe concisely the use of @ here, and how it relates to the existing namespace usage? In this case is it merely referring to the current namespace?

@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2015

@stefanpenner - This helper:@content-helper lookup is done by HTMLBars (I assume that it uses an @ to prevent collisions or something), so the @ here has nothing to do with the namespace concept that we have added to this resolver (which is why we are trying to detect invalid usage and falling back out of the "namespaced lookup" path).

@rwjblue
Copy link
Member

rwjblue commented Aug 5, 2015

I wrote a little more of a rambling explanation in #101 (comment) and emberjs/ember.js#11987.

@stefanpenner
Copy link
Contributor

@rwjblue thanks for the links, let me read the backchat.

@@ -57,6 +57,10 @@ define("ember/resolver",
var prefix, type, name;
var fullNameParts = fullName.split('@');

if (/.*:@.*/.test(fullName)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  • lets leave a comment here describing why this is needed
  • it seems like the above split fullName.split('@'); (on line 58) is not needed if we hit this branch, this should be cleaned up

@tim-evans
Copy link
Contributor Author

@stefanpenner Is the comment and changes I made sufficient?

@stefanpenner
Copy link
Contributor

@tim-evans yup, LG.

@rwjblue you are more familiar with that part of HTMLBars, so merge & release when you are comfortable with it.

@rwjblue
Copy link
Member

rwjblue commented Aug 6, 2015

OK, I did a bit more digging into what was happening and I'd prefer to specifically blacklist helper:@content-helper for now. It is a "magic" value (that @mmun is going to try and remove in HTMLBars), and the issue should not occur on canary already (because of cleanup work done to remove support for Ember.Handlebars.makeViewHelper).

@tim-evans - Can you tweak (hopefully for the last time) to just compare fullName === 'helper:@content-helper'?

@mmun
Copy link
Member

mmun commented Aug 6, 2015

Can't this change be made in htmlbars instead? @content-helper is an internal that shouldn't be leaking out...

…future release of htmlbars will remove this helper
@tim-evans
Copy link
Contributor Author

@rwjblue done.

@rwjblue
Copy link
Member

rwjblue commented Aug 6, 2015

Beautiful!

rwjblue added a commit that referenced this pull request Aug 6, 2015
Add failing test for @content-helper lookup
@rwjblue rwjblue merged commit 604c12d into ember-cli:master Aug 6, 2015
@mmun
Copy link
Member

mmun commented Aug 6, 2015

Ok rwjblue convinced me this is good

@tim-evans
Copy link
Contributor Author

Could we get a release on this? ember-document-title doesn't work on 1.13.X because of this. :(

I checked on 1.13.8 and it no longer throws errors when calling {{title}}, but calls the helper, which is not right at all.

@stefanpenner
Copy link
Contributor

releasing now.

@stefanpenner
Copy link
Contributor

➜  ember-resolver git:(master) git tag v0.1.19
➜  ember-resolver git:(master) git push origin v0.1.19
Total 0 (delta 0), reused 0 (delta 0)
To [email protected]:ember-cli/ember-resolver

@stefanpenner
Copy link
Contributor

@tim-evans let me know if that does the trick, sorry for letting this linger.

@tim-evans
Copy link
Contributor Author

Sadly, no. I'll come up with an ember-twiddle to report this.

@stefanpenner
Copy link
Contributor

@tim-evans ah so an additional bug in the resolver?

@tim-evans
Copy link
Contributor Author

@tim-evans
Copy link
Contributor Author

I'm not sure.

@stefanpenner
Copy link
Contributor

@tim-evans can you open a new issue, describing what is expected. I would love to dive in later this morning :)

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.

4 participants