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

[FEATURE ember-htmlbars-dashless-helpers] RFC#58. #11393

Merged
merged 1 commit into from
Jun 12, 2015

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jun 10, 2015

This implements the majority of emberjs/rfcs#58.

With this change, all helpers that are known to the registry or resolver are whitelisted automatically and are not subject to the former dash requirement.

Silly Demo: http://emberjs.jsbin.com/rwjblue/590/edit


@balinterdi
Copy link
Contributor

That's great! Does this also imply that helpers can have access to other services and components (in the general, not as in Ember components sense) of the application?

@balinterdi
Copy link
Contributor

Another consideration (I think) is that short helper names are still discouraged since it can cause conflict with bound values in templates. If I have a date helper and a date value (in a component's template), and I have {{date}} in the template, it will be considered as a helper and consequently I have no way to render the bound date value. Do I see this correctly?

@rwjblue
Copy link
Member Author

rwjblue commented Jun 10, 2015

Does this also imply that helpers can have access to other services and components (in the general, not as in Ember components sense) of the application?

That is possible via #11278 (discussed in emberjs/rfcs#53).

Another consideration (I think) is that short helper names are still discouraged since it can cause conflict with bound values in templates.

Agreed, a few points though:

  • That isn't a new issue (though this PR perhaps makes it easier) it was possible to do via Ember.Handlebars.registerHelper and other API's previously.
  • The deprecation of Ember.ObjectController makes things somewhat better (since you are very deliberately selecting the names for the properties in the downstream components).
  • It is just as likely to have a conflict for something like {{full-name}} (where that might be a component that deals with names), and that is also not a new issue.

I have no way to render the bound date value. Do I see this correctly?

Yes, a helper will "win" and override a local property. This is not a new change.

@balinterdi
Copy link
Contributor

True, thank you for your answers.

}

if (this.resolver.eachForType) {
this.resolver.eachForType(type, callback);
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

If you walk both the container and the resolver (or container and fallback), will you sometimes get duplicates? Things already loaded will show up twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it is theoretically possible for things to show up twice, but unlikely.

  • We only walk manually registered items in the registry (why manually register if it will auto-resolve?).
  • Book keeping to prevent double entry seems unneeded as we are keeping it in a small dictionary which will dedupe for us.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

ahha, I see. I did not realize it was only registered factories. Excellent.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 11, 2015

Submitted ember-cli/ember-resolver#95 to implement eachForType in the ember-cli resolver.

@rwjblue
Copy link
Member Author

rwjblue commented Jun 12, 2015

Update to match API with ember-cli/ember-resolver#95:

  • Changed eachForType to knownForType
  • The return value is now a simple object with the key as the container lookup name, and the value being true.

@rwjblue rwjblue force-pushed the dashless-helper-lookup branch 2 times, most recently from 4306884 to 7313fd3 Compare June 12, 2015 13:20
@rwjblue rwjblue added this to the 1.13.0 milestone Jun 12, 2015
@mixonic
Copy link
Sponsor Member

mixonic commented Jun 12, 2015

👏 Plz merge once the Travis gods have been sated. /cc @jamesarosen and others we know it is dreadfully short timing, but if you give us any feedback before too late in the afternoon eastern time that would be incredibly helpful.

mixonic added a commit that referenced this pull request Jun 12, 2015
[FEATURE ember-htmlbars-dashless-helpers] RFC#58.
@mixonic mixonic merged commit 9ae9522 into emberjs:master Jun 12, 2015
@mixonic mixonic deleted the dashless-helper-lookup branch June 12, 2015 14:40
@jamesarosen
Copy link

I'm at UX Burlington today, but I'll definitely try it out when I have a spare moment. Thanks so much for your work!

@jamesarosen
Copy link

Any idea when https://github.com/components/ember/tree/beta will get rebuilt?

@jamesarosen
Copy link

I just tried this out on the canary build. The helper registration works great for ember-i18n's helper:t :)

@rtm
Copy link

rtm commented Oct 9, 2015

I'm sort of frustrated at my inability to figure out how to implement a helper called _ in Ember 1.13.8. Ember.registerBoundHelper gives a deprecation message. Ember.Helper.helper('_', ... does not seem to work; I am told that "_" is not defined. Putting a _.js file in app/helpers and doing export default Ember.Helper.extend({... does not work. The documentation is decidedly unhelpful. What is the right way to do this?

@rwjblue
Copy link
Member Author

rwjblue commented Oct 9, 2015

Seems to be working here: http://ember-twiddle.com/53b8f0755d3feb4cf3ae

@rtm
Copy link

rtm commented Oct 10, 2015

Many thanks, got it working.

Now the issue is that in my unit tests for components, when _ is used directly (or indirectly?), I seem to add a needs: ['helper:_']. Is there any way to avoid that, which I would prefer, since there's more than a hundred of them. So there is now no alternative to putting the helpers in helpers/_.js, and no ability to just registering them directly? Note also that I have about a dozen additional translation-related helpers, and I would prefer to put them in one file instead of scattering them into a dozen files of a few lines each? By the way, would subdirectories in helpers be supported?

(By the way; this is a translation helper in the vein of t; _ is a sort of standard in some parts (eg gettext) for that).

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.

5 participants