Implement {{component}} helper #10093

Merged
merged 1 commit into from Jan 3, 2015

Conversation

Projects
None yet
@lukemelia
Member

lukemelia commented Dec 31, 2014

This PR implements the {{component}} helper, which allows passing of a param that is resolved and used to look up the component. Leveraging the first feature described above, it also automatically will swap the component for another when the bound value changes.

See #5007 for more discussion and background.

What's left:

  • first pass of implementation
  • collect code reviews and apply feedback
  • create {{component}} helper docs
  • basic test coverage
  • more code reviews and apply feedback
  • more test coverage, especially around actions
  • more code review and apply feedback?
@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Dec 31, 2014

Member

@mmun @krisselden @mixonic would love a review and feedback

Member

lukemelia commented Dec 31, 2014

@mmun @krisselden @mixonic would love a review and feedback

@igorT

View changes

packages/ember-htmlbars/lib/helpers/component.js
+ this.notify();
+ },
+ destroy: function(){
+ if (this._super$destroy()) {

This comment has been minimized.

@igorT

igorT Dec 31, 2014

Member

this._super$destroy doesn't seem to be defined?

@igorT

igorT Dec 31, 2014

Member

this._super$destroy doesn't seem to be defined?

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT Dec 31, 2014

Member

Might be nice to warn if 'ember-htmlbars-component-helper' is defined but 'ember-htmlbars-binding-aware-view-helper' is not

Member

igorT commented Dec 31, 2014

Might be nice to warn if 'ember-htmlbars-component-helper' is defined but 'ember-htmlbars-binding-aware-view-helper' is not

@igorT

View changes

packages/ember-views/lib/views/bound_view_view.js
+
+ var self = this;
+
+ this._boundViewOptions.viewStream.subscribe(function() {

This comment has been minimized.

@igorT

igorT Dec 31, 2014

Member

You seem to using a different style for subscribing here compared to component.js. Any reason we can't do
this._boundViewOptions.viewStream.subscribe(this._updateBoundChildView, this) ?

@igorT

igorT Dec 31, 2014

Member

You seem to using a different style for subscribing here compared to component.js. Any reason we can't do
this._boundViewOptions.viewStream.subscribe(this._updateBoundChildView, this) ?

@mixonic

View changes

packages/ember-htmlbars/lib/helpers/component.js
+ this.container = container;
+ if (isStream(source)) {
+ source.subscribe(this._didChange, this);
+ }

This comment has been minimized.

@mixonic

mixonic Dec 31, 2014

Member

You can use subscribe from utils: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/streams/utils.js#L7

In general the utils file is a set of helpers that are non-stream safe

@mixonic

mixonic Dec 31, 2014

Member

You can use subscribe from utils: https://github.com/emberjs/ember.js/blob/master/packages/ember-metal/lib/streams/utils.js#L7

In general the utils file is a set of helpers that are non-stream safe

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Dec 31, 2014

Member

Excited to see this feature get fleshed out :-) 👍 will review more.

Member

mixonic commented Dec 31, 2014

Excited to see this feature get fleshed out :-) 👍 will review more.

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT Dec 31, 2014

Member

Seems that using a ContainerView would mess with views that care about their parentView, though those are mostly components. If I am reading the PR correctly, using the component helper wouldn't create an intermediate component right?

Member

igorT commented Dec 31, 2014

Seems that using a ContainerView would mess with views that care about their parentView, though those are mostly components. If I am reading the PR correctly, using the component helper wouldn't create an intermediate component right?

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Dec 31, 2014

Member

Given that we will likely remove {{view}} or extract it into a plugin in 2.0, I'm 👎 on adding new functionality to it. {{component}} helper seems cool though.

Member

tomdale commented Dec 31, 2014

Given that we will likely remove {{view}} or extract it into a plugin in 2.0, I'm 👎 on adding new functionality to it. {{component}} helper seems cool though.

@mixonic

View changes

packages/ember-htmlbars/lib/helpers/component.js
+ "The `component` helper expects exactly one argument, plus name/property values.",
+ params.length === 1
+ );
+ var container = this.container || this._keywords.view.value().container;

This comment has been minimized.

@mixonic

mixonic Dec 31, 2014

Member

Again I'd suggest read instead of accessing value()

@mixonic

mixonic Dec 31, 2014

Member

Again I'd suggest read instead of accessing value()

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Dec 31, 2014

Member

Agreed w/ @tomdale re: new view functionality.

Member

mixonic commented Dec 31, 2014

Agreed w/ @tomdale re: new view functionality.

@mixonic

View changes

packages/ember-htmlbars/lib/helpers/component.js
+import { read, isStream } from "ember-metal/streams/utils";
+import EmberError from "ember-metal/error";
+
+function ComponentLookupStream(source, componentLookup, container) {

This comment has been minimized.

@mixonic

mixonic Dec 31, 2014

Member

I'd like to suggest you explore chainStream for this logic instead of making a custom stream. chainStream, like read is non-stream safe.

Values get into the chain via scope instead of an object instance.

var component = chainStream(source, function(){
  var componentParam = read(source);
  return componentLookup.lookupFactory(componentParam, container);
});
// component is maybe a component, maybe a stream of component values

Because both the utils are non-stream safe, you don't even need to branch the initial logic based on if this is a stream.

I think chainSteam could greatly reduce the amount of code required to implement this feature.

@mmun also mentions that lookupComponent could possibly return a stream itself, instead of wrapping it.

@mixonic

mixonic Dec 31, 2014

Member

I'd like to suggest you explore chainStream for this logic instead of making a custom stream. chainStream, like read is non-stream safe.

Values get into the chain via scope instead of an object instance.

var component = chainStream(source, function(){
  var componentParam = read(source);
  return componentLookup.lookupFactory(componentParam, container);
});
// component is maybe a component, maybe a stream of component values

Because both the utils are non-stream safe, you don't even need to branch the initial logic based on if this is a stream.

I think chainSteam could greatly reduce the amount of code required to implement this feature.

@mmun also mentions that lookupComponent could possibly return a stream itself, instead of wrapping it.

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Dec 31, 2014

Contributor

@tomdale It would be a shame to loose {{view}}. I know people are generally encouraged to use {{component}}, but we frequently have tiny little one off view classes that live on the parentView. They're not meant to ever be reused and have a tightly coupled relationship with the parent view, they exist for one purpose. We could make them components, but our list of components would be a lot longer and less organized.

Just my $0.02.

Contributor

workmanw commented Dec 31, 2014

@tomdale It would be a shame to loose {{view}}. I know people are generally encouraged to use {{component}}, but we frequently have tiny little one off view classes that live on the parentView. They're not meant to ever be reused and have a tightly coupled relationship with the parent view, they exist for one purpose. We could make them components, but our list of components would be a lot longer and less organized.

Just my $0.02.

@igorT

This comment has been minimized.

Show comment
Hide comment
@igorT

igorT Dec 31, 2014

Member

Components now support / in names so you can organize them much easier.

Member

igorT commented Dec 31, 2014

Components now support / in names so you can organize them much easier.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Dec 31, 2014

Member

@workmanw We will support coupled components without needing the {{view}} helper.

Member

mmun commented Dec 31, 2014

@workmanw We will support coupled components without needing the {{view}} helper.

@rwjblue

View changes

packages/ember-htmlbars/lib/helpers/view.js
@@ -199,17 +205,23 @@ export function viewHelper(params, hash, options, env) {
viewClassOrInstance = View;
}
} else {
- viewClassOrInstance = readViewFactory(params[0], container);
+ var viewParam = params[0];
+ if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper') && isStream(viewParam)) {

This comment has been minimized.

@rwjblue

rwjblue Dec 31, 2014

Member

Feature flag checks have to be by themselves for them to be properly stripped by defeatureify. This would have to be rewritten as:

if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper')) {
  if (isStream(viewParam)) {
    /* stuff */
  }
}
@rwjblue

rwjblue Dec 31, 2014

Member

Feature flag checks have to be by themselves for them to be properly stripped by defeatureify. This would have to be rewritten as:

if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper')) {
  if (isStream(viewParam)) {
    /* stuff */
  }
}

This comment has been minimized.

@mixonic

mixonic Dec 31, 2014

Member

Thanks for catching this! I was thinking it was busted.

@mixonic

mixonic Dec 31, 2014

Member

Thanks for catching this! I was thinking it was busted.

@rwjblue

View changes

packages/ember-htmlbars/lib/main.js
@@ -59,6 +60,9 @@ import "ember-htmlbars/system/bootstrap";
import "ember-htmlbars/compat";
registerHelper('view', viewHelper);
+if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper') && Ember.FEATURES.isEnabled('ember-htmlbars-component-helper')) {

This comment has been minimized.

@rwjblue

rwjblue Dec 31, 2014

Member

Feature flag checks must be by themselves, and sadly cannot be non-nested. To do this behavior you would need something like:

var bindingAwareViewHelper, componentHelper;
if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper')) {
  bindingAwareViewHelper = true;
}

if (Ember.FEATURES.isEnabled('ember-htmlbars-component-helper')) {
  componentHelper = true;
}

if (bindingAwareViewHelper && componentHelper) {
  /* stuff */
}

(yes, it is annoying, I am sorry)

@rwjblue

rwjblue Dec 31, 2014

Member

Feature flag checks must be by themselves, and sadly cannot be non-nested. To do this behavior you would need something like:

var bindingAwareViewHelper, componentHelper;
if (Ember.FEATURES.isEnabled('ember-htmlbars-binding-aware-view-helper')) {
  bindingAwareViewHelper = true;
}

if (Ember.FEATURES.isEnabled('ember-htmlbars-component-helper')) {
  componentHelper = true;
}

if (bindingAwareViewHelper && componentHelper) {
  /* stuff */
}

(yes, it is annoying, I am sorry)

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jan 1, 2015

Member

Thanks for all the great feedback. I'm laid up with a cold today, but look forward to taking another pass.

Member

lukemelia commented Jan 1, 2015

Thanks for all the great feedback. I'm laid up with a cold today, but look forward to taking another pass.

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Jan 1, 2015

Contributor

@mmun I'm sorry, I don't understand what you mean. Are you saying, {{component boundComponentClass}} would be supported? If not, would you mind elaborating?

Contributor

workmanw commented Jan 1, 2015

@mmun I'm sorry, I don't understand what you mean. Are you saying, {{component boundComponentClass}} would be supported? If not, would you mind elaborating?

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 1, 2015

Member

@workmanw - Yes, {{component someBoundProp}} should absolutely work (and is what this PR is all about).

Member

rwjblue commented Jan 1, 2015

@workmanw - Yes, {{component someBoundProp}} should absolutely work (and is what this PR is all about).

@workmanw

This comment has been minimized.

Show comment
Hide comment
@workmanw

workmanw Jan 1, 2015

Contributor

@rwjblue Awesome. Thanks a lot. It wasn't clear to me if "someBoundProp" could be a class object as well as a string name.

Contributor

workmanw commented Jan 1, 2015

@rwjblue Awesome. Thanks a lot. It wasn't clear to me if "someBoundProp" could be a class object as well as a string name.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 1, 2015

Member

Should be a string name to be looked up in the container.

Member

rwjblue commented Jan 1, 2015

Should be a string name to be looked up in the container.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Jan 1, 2015

Member

@workmanw I mean that we won't deprecate the view helper until there are well defined upgrade paths. Localized / scoped components is something we've been thinking about for a while but nothing concrete yet. Expect an RFC before EmberConf.

Member

mmun commented Jan 1, 2015

@workmanw I mean that we won't deprecate the view helper until there are well defined upgrade paths. Localized / scoped components is something we've been thinking about for a while but nothing concrete yet. Expect an RFC before EmberConf.

@lukemelia lukemelia changed the title from Add bindable view support to {{view}} helper and implement {{component}} helper to Implement {{component}} helper Jan 1, 2015

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jan 2, 2015

Member

@igorT @mmun @mixonic @rwjblue @tomdale Just pushed a second pass, removing the new view feature and applying the excellent feedback. A few questions:

  1. lookupHelper is used in the base of typical component rendering, and it delegates to the view helper. (https://github.com/emberjs/ember.js/blob/master/packages/ember-htmlbars/lib/system/lookup-helper.js#L51). I have maintained that pattern for the {{component}} helper, but am open to another approach if that is not desirable.
  2. The intermediate BoundViewView that is being created in this PR's current incarnation is a ContainerView subclass. Is this acceptable, or should this be a component? I'm not sure of the pros and cons to each approach.
Member

lukemelia commented Jan 2, 2015

@igorT @mmun @mixonic @rwjblue @tomdale Just pushed a second pass, removing the new view feature and applying the excellent feedback. A few questions:

  1. lookupHelper is used in the base of typical component rendering, and it delegates to the view helper. (https://github.com/emberjs/ember.js/blob/master/packages/ember-htmlbars/lib/system/lookup-helper.js#L51). I have maintained that pattern for the {{component}} helper, but am open to another approach if that is not desirable.
  2. The intermediate BoundViewView that is being created in this PR's current incarnation is a ContainerView subclass. Is this acceptable, or should this be a component? I'm not sure of the pros and cons to each approach.
@rwjblue

View changes

packages/ember-htmlbars/lib/helpers/component.js
+import EmberError from "ember-metal/error";
+
+/**
+ `{{component}}` is a `Ember.Handlebars` helper for adding instances of

This comment has been minimized.

@rwjblue

rwjblue Jan 2, 2015

Member

Ember.HTMLBars or "template" helper? Once 1.10 lands Ember.Handlebars is for backwards compat only (and Handlebars itself isn't actually used/needed)...

@rwjblue

rwjblue Jan 2, 2015

Member

Ember.HTMLBars or "template" helper? Once 1.10 lands Ember.Handlebars is for backwards compat only (and Handlebars itself isn't actually used/needed)...

@rwjblue

View changes

packages/ember-htmlbars/lib/helpers/component.js
+ var componentOrStream = readComponentFactory(componentParam, container);
+
+ if (componentOrStream) {
+ env.helpers.view.helperFunction.call(this, [componentOrStream], hash, options, env);

This comment has been minimized.

@rwjblue

rwjblue Jan 2, 2015

Member

I think that we would prefer to avoid the "helpers calling helpers" thing, I believe all we need to do is:

import mergeViewBindings from "ember-htmlbars/system/merge-view-bindings";
import appendTemplatedView from "ember-htmlbars/system/append-templated-view";

var props = {
    helperName: options.helperName || 'component'
};
mergeViewBindings(this, props, hash);
appendTemplatedView(this, options.morph, componentOrStream, props);

(from here)

But we should wait for @mmun and @mixonic to confirm...

@rwjblue

rwjblue Jan 2, 2015

Member

I think that we would prefer to avoid the "helpers calling helpers" thing, I believe all we need to do is:

import mergeViewBindings from "ember-htmlbars/system/merge-view-bindings";
import appendTemplatedView from "ember-htmlbars/system/append-templated-view";

var props = {
    helperName: options.helperName || 'component'
};
mergeViewBindings(this, props, hash);
appendTemplatedView(this, options.morph, componentOrStream, props);

(from here)

But we should wait for @mmun and @mixonic to confirm...

This comment has been minimized.

@mixonic

mixonic Jan 2, 2015

Member

Doing this is definitely not good. helperName is only there for debugging purposes. In general we do want to decouple helpers and stop manipulating args to pass to another helper. We might be missing a utility method.

@mixonic

mixonic Jan 2, 2015

Member

Doing this is definitely not good. helperName is only there for debugging purposes. In general we do want to decouple helpers and stop manipulating args to pass to another helper. We might be missing a utility method.

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 2, 2015

Member

Can you add a test for this case: https://github.com/emberjs/ember.js/pull/10093/files#diff-7c2e9f44db2fa5bee304255d11753999R72 (when the value returned by readComponentFactory is undefined)?

Member

rwjblue commented Jan 2, 2015

Can you add a test for this case: https://github.com/emberjs/ember.js/pull/10093/files#diff-7c2e9f44db2fa5bee304255d11753999R72 (when the value returned by readComponentFactory is undefined)?

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jan 2, 2015

Member

Latest is up. I'm guessing actions don't work yet. Next stop is tests for that.

Member

lukemelia commented Jan 2, 2015

Latest is up. I'm guessing actions don't work yet. Next stop is tests for that.

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jan 2, 2015

Member

Looks like actions are in good shape after all. Now has a test. Can you guys think of other things we should be testing for this?

Member

lukemelia commented Jan 2, 2015

Looks like actions are in good shape after all. Now has a test. Can you guys think of other things we should be testing for this?

@rlivsey

This comment has been minimized.

Show comment
Hide comment
@rlivsey

rlivsey Jan 2, 2015

Contributor

👍 awesome stuff, looking great!

As far as I can tell it covers everything I'm using my {{dynamic-component}} helper for (but far better implemented).

I'll see if I can find some time later today to give it a whirl in my app

Contributor

rlivsey commented Jan 2, 2015

👍 awesome stuff, looking great!

As far as I can tell it covers everything I'm using my {{dynamic-component}} helper for (but far better implemented).

I'll see if I can find some time later today to give it a whirl in my app

+import mergeViewBindings from "ember-htmlbars/system/merge-view-bindings";
+import EmberError from "ember-metal/error";
+
+export default Ember.ContainerView.extend(_Metamorph, {

This comment has been minimized.

@rwjblue

rwjblue Jan 2, 2015

Member

Since we only have a single child we do not need the power of ContainerView (to manage arrays of children). I think this can be done with a normal Ember.View (instead of a Ember.ContainerView).

I created a JSBin here with a tiny demo (would still need the _boundComponentProps stuff you have). The basic gist is that adding a child view in render is cleared for every rerender. You could extend packages/ember-views/lib/mixins/normalized_rerender_if_needed.js and set the componentNameStream at lazyValue (like BoundPartialView).

@mmun / @mixonic - Does you agree?

@rwjblue

rwjblue Jan 2, 2015

Member

Since we only have a single child we do not need the power of ContainerView (to manage arrays of children). I think this can be done with a normal Ember.View (instead of a Ember.ContainerView).

I created a JSBin here with a tiny demo (would still need the _boundComponentProps stuff you have). The basic gist is that adding a child view in render is cleared for every rerender. You could extend packages/ember-views/lib/mixins/normalized_rerender_if_needed.js and set the componentNameStream at lazyValue (like BoundPartialView).

@mmun / @mixonic - Does you agree?

This comment has been minimized.

@lukemelia

lukemelia Jan 2, 2015

Member

@rwjblue I'm open to this approach. I'm curious about what the expected gain is, though. Is it faster or cheaper (in terms of memory)? It seems to me that what we are doing here is very similar what OutletView does, which is implemented as ContainerView.extend(_Metamorph).

On that note, I wonder what consideration we should give to extension points for animation (e.g. LiquidFire). @ef4 or others familiar with it?

@lukemelia

lukemelia Jan 2, 2015

Member

@rwjblue I'm open to this approach. I'm curious about what the expected gain is, though. Is it faster or cheaper (in terms of memory)? It seems to me that what we are doing here is very similar what OutletView does, which is implemented as ContainerView.extend(_Metamorph).

On that note, I wonder what consideration we should give to extension points for animation (e.g. LiquidFire). @ef4 or others familiar with it?

This comment has been minimized.

@rwjblue

rwjblue Jan 2, 2015

Member

Updated the JSBin to share the same morph (thanks to @mmun for pointing out the extra morhps being created in the first one).

@rwjblue

rwjblue Jan 2, 2015

Member

Updated the JSBin to share the same morph (thanks to @mmun for pointing out the extra morhps being created in the first one).

This comment has been minimized.

@rwjblue

rwjblue Jan 2, 2015

Member

@lukemelia - Ember.ContainerView does much more than what we need (and I think that OutletView should likely be updated as well). For example ContainerView calls defineProperty on itself during init (which is generally known to be a perf issue).

I'm not unhappy with your current implementation, I just think it could be better/simpler. I'm happy to wait for the real view experts (obviously not me) to chime in...

@rwjblue

rwjblue Jan 2, 2015

Member

@lukemelia - Ember.ContainerView does much more than what we need (and I think that OutletView should likely be updated as well). For example ContainerView calls defineProperty on itself during init (which is generally known to be a perf issue).

I'm not unhappy with your current implementation, I just think it could be better/simpler. I'm happy to wait for the real view experts (obviously not me) to chime in...

This comment has been minimized.

@lukemelia

lukemelia Jan 2, 2015

Member

@rwjblue makes sense. I'll stay tuned.

@lukemelia

lukemelia Jan 2, 2015

Member

@rwjblue makes sense. I'll stay tuned.

This comment has been minimized.

@igorT

igorT Jan 2, 2015

Member

Will using a View instead of ContainerView fix the parentView problem?

@igorT

igorT Jan 2, 2015

Member

Will using a View instead of ContainerView fix the parentView problem?

This comment has been minimized.

@lukemelia

lukemelia Jan 2, 2015

Member

@igorT I don't think there is a parentView problem in the current implementation, unless I am misunderstanding. BoundComponentView is virtual, so it is skipped in the parentView getter.

Console

I'll add an assertion to make sure this remains true.

@lukemelia

lukemelia Jan 2, 2015

Member

@igorT I don't think there is a parentView problem in the current implementation, unless I am misunderstanding. BoundComponentView is virtual, so it is skipped in the parentView getter.

Console

I'll add an assertion to make sure this remains true.

This comment has been minimized.

@lukemelia

lukemelia Jan 2, 2015

Member

Added a new test to confirm this in the latest revision. Let me know if this is not what you meant @igorT

@lukemelia

lukemelia Jan 2, 2015

Member

Added a new test to confirm this in the latest revision. Let me know if this is not what you meant @igorT

This comment has been minimized.

@igorT

igorT Jan 9, 2015

Member

👍

@igorT

igorT Jan 9, 2015

Member

👍

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Jan 2, 2015

Member

@lukemelia great work. I plan to do a thorough review soon.

Member

mmun commented Jan 2, 2015

@lukemelia great work. I plan to do a thorough review soon.

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Jan 2, 2015

Member

We discussed this at the weekly meeting and reached consensus that this is good to merge once @mmun finishes his review.

Member

tomdale commented Jan 2, 2015

We discussed this at the weekly meeting and reached consensus that this is good to merge once @mmun finishes his review.

+@module ember
+@submodule ember-htmlbars
+*/
+import Ember from "ember-metal/core"; // Ember.warn, Ember.assert

This comment has been minimized.

@stefanpenner

stefanpenner Jan 2, 2015

Member

maybe flagged aswell so we don't pull in the extra bytes

@stefanpenner

stefanpenner Jan 2, 2015

Member

maybe flagged aswell so we don't pull in the extra bytes

This comment has been minimized.

@lukemelia

lukemelia Jan 2, 2015

Member

I don't follow. Do you have an example of this, @stefanpenner?

@lukemelia

lukemelia Jan 2, 2015

Member

I don't follow. Do you have an example of this, @stefanpenner?

[FEATURE ember-htmlbars-component-helper] Implement `{{component path…
….resolving.to.component.name}}` to dynamically update the component when the bound value changes.

mmun added a commit that referenced this pull request Jan 3, 2015

@mmun mmun merged commit eff1014 into emberjs:master Jan 3, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@tobscure

This comment has been minimized.

Show comment
Hide comment
@tobscure

tobscure Jan 3, 2015

Couple of questions/thoughts:

  • Does the helper work with a component class object or instance, like the view helper did?
  • Does it mean that we can easily use components named without a hyphen? e.g. {{component "button"}}

tobscure commented Jan 3, 2015

Couple of questions/thoughts:

  • Does the helper work with a component class object or instance, like the view helper did?
  • Does it mean that we can easily use components named without a hyphen? e.g. {{component "button"}}
@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Jan 3, 2015

Member

Does the helper work with a component class object or instance, like the view helper did?

both

Does it mean that we can easily use components named without a hyphen? e.g. {{component "button"}}

yes

Member

stefanpenner commented Jan 3, 2015

Does the helper work with a component class object or instance, like the view helper did?

both

Does it mean that we can easily use components named without a hyphen? e.g. {{component "button"}}

yes

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Jan 3, 2015

Member

@stefanpenner We should enforce component lookup via strings and disallow classes and instances, no? I don't want people writing inline component classes inside of their components until the story has been fleshed out.

Member

mmun commented Jan 3, 2015

@stefanpenner We should enforce component lookup via strings and disallow classes and instances, no? I don't want people writing inline component classes inside of their components until the story has been fleshed out.

@bcardarella

This comment has been minimized.

Show comment
Hide comment
@bcardarella

bcardarella Jan 3, 2015

Contributor

Perhaps I am missing the obvious but what advantage does this have over:

{{#if isMarketOpen}}
  {{live-updating-chart}}
{{else}}
  {{market-close-summary}}
{{/if}}

Functionally it appears this implementation does exactly the same but adds additional code to Ember.

Contributor

bcardarella commented Jan 3, 2015

Perhaps I am missing the obvious but what advantage does this have over:

{{#if isMarketOpen}}
  {{live-updating-chart}}
{{else}}
  {{market-close-summary}}
{{/if}}

Functionally it appears this implementation does exactly the same but adds additional code to Ember.

@twokul

This comment has been minimized.

Show comment
Hide comment
@twokul

twokul Jan 3, 2015

Member

@bcardarella if I'm not mistaken, this will allow to pass a component name to the helper dynamically. So you will avoid the need to have if statements in your template. Say something like:

{{select content=chartTypes selection=chartType}}
{{component chartType}}

where there's a select with different chart types and as soon as you select a different type, a new component is rendered (i.e. the selection is barChart, bar-chart-component will be rendered, etc.)

Member

twokul commented Jan 3, 2015

@bcardarella if I'm not mistaken, this will allow to pass a component name to the helper dynamically. So you will avoid the need to have if statements in your template. Say something like:

{{select content=chartTypes selection=chartType}}
{{component chartType}}

where there's a select with different chart types and as soon as you select a different type, a new component is rendered (i.e. the selection is barChart, bar-chart-component will be rendered, etc.)

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 3, 2015

Member

@bcardarella - there are many cases where the actual component name to use is determined at runtime (like from a model property). This was not previously possible out of the box for components.

Member

rwjblue commented Jan 3, 2015

@bcardarella - there are many cases where the actual component name to use is determined at runtime (like from a model property). This was not previously possible out of the box for components.

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Jan 3, 2015

Member

Does the helper work with a component class object or instance, like the view helper did?

@tobscure @stefanpenner @mmun Test coverage only covers name lookups, and IIRC, component class references are not supported per discussions during the issue/PR process. It would be trivial to support class references, but my understanding from @mmun is that we want to come up with another approach to support those use cases.

Member

lukemelia commented Jan 3, 2015

Does the helper work with a component class object or instance, like the view helper did?

@tobscure @stefanpenner @mmun Test coverage only covers name lookups, and IIRC, component class references are not supported per discussions during the issue/PR process. It would be trivial to support class references, but my understanding from @mmun is that we want to come up with another approach to support those use cases.

@lukemelia lukemelia deleted the lukemelia:helper-binding-support branch Jan 3, 2015

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 3, 2015

Member

It would be trivial to support class references, but my understanding from @mmun is that we want to come up with another approach to support those use cases.

Confirm, we only want to support strings to be looked up in the container (not component instances or classes) at this point. As you said, it would be possible to add support for instances and classes, but we definitely do not want to start with so much extra API surface.

Member

rwjblue commented Jan 3, 2015

It would be trivial to support class references, but my understanding from @mmun is that we want to come up with another approach to support those use cases.

Confirm, we only want to support strings to be looked up in the container (not component instances or classes) at this point. As you said, it would be possible to add support for instances and classes, but we definitely do not want to start with so much extra API surface.

@jasonmit jasonmit referenced this pull request in minutebase/ember-dynamic-component Jan 4, 2015

Closed

Add deprecation warning once {{component}} PR lands in a stable release #4

@rwjblue

This comment has been minimized.

Show comment
Hide comment
@rwjblue

rwjblue Jan 11, 2015

Member

As of #10193 this is enabled by default on canary (and slated for 1.11 barring any issues).

Member

rwjblue commented Jan 11, 2015

As of #10193 this is enabled by default on canary (and slated for 1.11 barring any issues).

@taras taras referenced this pull request in EmberSherpa/ember-helpers-render-component Jan 31, 2015

Closed

Ember 2.0 #1

@shinzui

This comment has been minimized.

Show comment
Hide comment
@shinzui

shinzui Mar 5, 2015

How are you supposed to pass data to your component while using this helper?

shinzui commented Mar 5, 2015

How are you supposed to pass data to your component while using this helper?

@lukemelia

This comment has been minimized.

Show comment
Hide comment
@lukemelia

lukemelia Mar 5, 2015

Member

@shinzui name value pairs, just like on your components, see tests for an example (https://github.com/emberjs/ember.js/pull/10093/files#diff-de0c559d2c25a9811bd9876f63269098R36)

Member

lukemelia commented Mar 5, 2015

@shinzui name value pairs, just like on your components, see tests for an example (https://github.com/emberjs/ember.js/pull/10093/files#diff-de0c559d2c25a9811bd9876f63269098R36)

@shinzui

This comment has been minimized.

Show comment
Hide comment
@shinzui

shinzui Mar 5, 2015

@lukemelia great, thanks. I asked based on reading the code, i should have tried it.

shinzui commented Mar 5, 2015

@lukemelia great, thanks. I asked based on reading the code, i should have tried it.

hlm pushed a commit to hlm/sl-ember-components that referenced this pull request May 19, 2015

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Jun 16, 2015

Contributor

Are there plans to enhance this to allow passing component classes? The discussion above mentions this functionality, but I don't see it in the commit.

It would be useful for 'private' components, that are only used within a parent component (which they are coupled with).

Contributor

sandstrom commented Jun 16, 2015

Are there plans to enhance this to allow passing component classes? The discussion above mentions this functionality, but I don't see it in the commit.

It would be useful for 'private' components, that are only used within a parent component (which they are coupled with).

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Jun 16, 2015

Contributor

@sandstrom see emberjs/rfcs#64

I think that is the solution for private child components.

Contributor

ef4 commented Jun 16, 2015

@sandstrom see emberjs/rfcs#64

I think that is the solution for private child components.

@sandstrom

This comment has been minimized.

Show comment
Hide comment
@sandstrom

sandstrom Jun 16, 2015

Contributor

@ef4 thanks!

Contributor

sandstrom commented Jun 16, 2015

@ef4 thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment