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

Implement Ember.Helper: RFC#53. #11278

Merged
merged 4 commits into from
Jun 8, 2015
Merged

Implement Ember.Helper: RFC#53. #11278

merged 4 commits into from
Jun 8, 2015

Conversation

mixonic
Copy link
Sponsor Member

@mixonic mixonic commented May 25, 2015

Implements emberjs/rfcs#53, new helpers.

For example:

// app/helpers/full-name.js
import Ember from "ember";

export default Ember.Helper.extend({
  nameBuilder: Ember.inject.service(),
  compute(params) {
    const builder = this.get('nameBuilder');
    return builder.fullName(params[0], params[1]);
  }
});

TODO:

  • When looking up a helper in dev mode, catch any error about a bare function being registered into the container and rephrase it to be more kind: A bare function was resolved as a helper: This is not supported. Please upgrade your resolver, or use "Ember.Helper.build"
  • Create an ember-helper package?
  • Can the class based helpers manage their own lifecycle (use the same instance across multiple param and hash updates)?
  • export the Ember.Helper global
  • test usage in subexpressions
  • test assertion when used with a block
  • revert to lookupFactory instead of lookup for helpers
  • rebase/squash
  • test calling recompute on b in (a (b (c)))
  • test teardown of b in (a (b (c)))

A followup PR should deprecate these legacy APIs in 1.13.1:

  • Ember.Handlebars.makeBoundHelper
  • Ember.HTMLBars.makeBoundHelper (private)
  • bare functions returned as helpers (HandlebarsCompatibleHelper)

@mmun
Copy link
Member

mmun commented May 26, 2015

Can you add a test that uses a service so we know the container is hooked up correctly?

@mixonic
Copy link
Sponsor Member Author

mixonic commented May 26, 2015

Tests are green (pending sauce), and I've addressed two items of @mmun feedback:

  • Add a test for DI
  • Move the helper stream out of getStream and into invokeHelper

@mmun
Copy link
Member

mmun commented May 26, 2015

Yay :)

@@ -1003,7 +1003,7 @@ Application.reopenClass({
registry.optionsForType('component', { singleton: false });
registry.optionsForType('view', { singleton: false });
registry.optionsForType('template', { instantiate: false });
registry.optionsForType('helper', { instantiate: false });
registry.optionsForType('helper', { singleton: false });
Copy link
Member

Choose a reason for hiding this comment

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

I think the singleton part isn't that important. The rest of the system should likely prefer lookupFactory(helperName).create(..) anyways ?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Yep, per the TODO list I'm going to move to using lookupFactory.

Copy link
Member

Choose a reason for hiding this comment

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

👍

@raytiley
Copy link
Contributor

Sorry I'm late to the party here. I see this example in the RFC

// app/helpers/can-access.js
import Ember from "ember";

// Usage {{if (can-access 'admin') 'Welcome, boss' 'Heck no!'}}
export default Ember.Helper.extend({
  session: Ember.inject.service(),
  onCurrentUserChange: Ember.observes('session.currentUser', function() {
    this.recompute();
  }),
  compute(params) {
    const currentUser = this.get('session.currentUser');
    return currentUser.can(params[0]);
  }
});

I know that under the hood somehow this will use observers but should it be more abstracted so I can avoid owing @stefanpenner 10 bucks each time I write a Helper?

Maybe an array property that under the hood will setup an observer and call recompute so that the helper would be written like this:

// app/helpers/can-access.js
import Ember from "ember";

// Usage {{if (can-access 'admin') 'Welcome, boss' 'Heck no!'}}
export default Ember.Helper.extend({
  session: Ember.inject.service(),
  dependentKeys: ['session.currentUser'], // When this path changes recompute() is called
  compute(params) {
    const currentUser = this.get('session.currentUser');
    return currentUser.can(params[0]);
  }
});

@mixonic
Copy link
Sponsor Member Author

mixonic commented May 28, 2015

@raytiley an earlier version of the RFC proposed that same API under the property recomputeObserves. It was removed with the goal of making helpers as conservative and learnable as possible. It, and other enhancements, can always be added later.

recompute is safe to call many times, and will only ever issue a single render. Even if your observer is firing eagerly, and runs several times, the work in compute is lazy and will only happen once.

@@ -34,13 +34,13 @@ QUnit.module("ember-htmlbars: compat - Ember.Handlebars.get", {
QUnit.test('it can lookup a path from the current context', function() {
expect(1);

registry.register('helper:handlebars-get', function(path, options) {
registry.register('helper:handlebars-get', new HandlebarsCompatibleHelper(function(path, options) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems like this manual wrapping should not be needed. If it is required does that mean we have broken exporting Handlebars style helpers from app/helpers/foo-bar.js?

Copy link
Member

Choose a reason for hiding this comment

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

After discussing with @mixonic, this is needed because we are wrapping eagerly when a helper is resolved (in the resolver changes in this PR). In this case, we are bypassing the resolver so this wrapping is required.

@cibernox
Copy link
Contributor

cibernox commented Jun 6, 2015

quick update: Is there any chance of this making it into 1.13?

@mixonic
Copy link
Sponsor Member Author

mixonic commented Jun 6, 2015

@cibernox si, it will go into 1.13. Just discussed it again at the f2f. I have one or two small changes but no more blockers, will land it by Sunday eve.

@cibernox
Copy link
Contributor

cibernox commented Jun 6, 2015

happy

@rwjblue
Copy link
Member

rwjblue commented Jun 6, 2015

@cibernox - Yes, it is absolutely critical that we land this into 1.13.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Jun 7, 2015

Ember.Helper is now feature complete.

I have punted on the ideal life-cycle for class-based helpers. The ideal is that an instance of a class-based helper lasts as long as its value is rendered on the page (even as that value changes), but is destroyed when the value is removed from rendering. Here, the instance is created and destroyed for every computation. This will likely change, but may not change until after 1.13 ships. This fact should not stop anyone from using these APIs.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Jun 7, 2015

Will require tildeio/htmlbars#364 and a release for HTMLBars

@rwjblue rwjblue changed the title WIP: Helpers Helpers Jun 7, 2015
@rwjblue rwjblue changed the title Helpers Implement Ember.Helper: RFC#53. Jun 7, 2015
// Ember.Helper.build(function(params, hash) {});
Helper.reopenClass({
isHelperFactory: true,
build(helperFn) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you:

  • rename this from build to helper
  • move it out of reopenClass (because we don't extended class to have a helper on their factories)
  • export a named helper function (for future ease in importing)

@cibernox
Copy link
Contributor

cibernox commented Jun 7, 2015

Should I update the blueprint in ember-cli?

I assume that the shorthand function only helper is the preferred approach (for simplicity), but maybe adding a comment with an example how a full-fledged helper is a good idea.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Jun 8, 2015

@cibernox I think a PR for that would be fantastic. I'm not sure what the right timing is to merge it into Ember-CLI, but the sooner we can get people off the old APIs the better.

mixonic added a commit that referenced this pull request Jun 8, 2015
Implement Ember.Helper: RFC#53.
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.

7 participants