Implement {{component}} helper #10093

lukemelia · 2014-12-31T08:12:06Z

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 · 2014-12-31T08:12:59Z

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

igorT · 2014-12-31T14:20:56Z

packages/ember-htmlbars/lib/helpers/component.js

+    this.notify();
+  },
+  destroy: function(){
+    if (this._super$destroy()) {


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

igorT · 2014-12-31T14:27:22Z

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

igorT · 2014-12-31T14:40:08Z

packages/ember-views/lib/views/bound_view_view.js

+
+    var self = this;
+
+    this._boundViewOptions.viewStream.subscribe(function() {


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 · 2014-12-31T15:06:38Z

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

igorT · 2014-12-31T15:24:45Z

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 · 2014-12-31T15:41:30Z

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 · 2014-12-31T16:42:51Z

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;


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

mixonic · 2014-12-31T16:54:28Z

Agreed w/ @tomdale re: new view functionality.

mixonic · 2014-12-31T17:04:16Z

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) {


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 · 2014-12-31T18:28:02Z

@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 · 2014-12-31T18:29:58Z

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

mmun · 2014-12-31T18:41:18Z

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

rwjblue · 2014-12-31T23:25:59Z

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)) {


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 */ } }

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

lukemelia · 2015-01-01T05:31:28Z

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

workmanw · 2015-01-01T16:13:55Z

@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 · 2015-01-01T17:29:14Z

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

workmanw · 2015-01-01T18:36:50Z

@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 · 2015-01-01T19:19:28Z

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

mmun · 2015-01-01T20:10:25Z

@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 · 2015-01-02T00:09:17Z

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

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.
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 · 2015-01-02T01:01:43Z

packages/ember-htmlbars/lib/helpers/component.js

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


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

mmun · 2015-01-02T18:35:57Z

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

tomdale · 2015-01-02T19:22:46Z

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

stefanpenner · 2015-01-02T19:23:23Z

packages/ember-htmlbars/lib/helpers/component.js

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


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

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

….resolving.to.component.name}}` to dynamically update the component when the bound value changes.

Implement {{component}} helper

tobyzerner · 2015-01-03T05:59:32Z

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 · 2015-01-03T06:49:50Z

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 · 2015-01-03T08:02:44Z

@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 · 2015-01-03T09:05:46Z

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 · 2015-01-03T09:55:39Z

@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 · 2015-01-03T12:59:51Z

@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 · 2015-01-03T19:43:48Z

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.

rwjblue · 2015-01-03T20:14:28Z

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.

rwjblue · 2015-01-11T22:53:33Z

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

shinzui · 2015-03-05T01:20:19Z

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

lukemelia · 2015-03-05T01:34:03Z

@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 · 2015-03-05T01:37:54Z

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

sandstrom · 2015-06-16T13:52:26Z

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 · 2015-06-16T13:58:38Z

@sandstrom see emberjs/rfcs#64

I think that is the solution for private child components.

sandstrom · 2015-06-16T18:18:26Z

@ef4 thanks!

lukemelia mentioned this pull request Dec 31, 2014

consider a component helper for dynamic component loading. #5007

Closed

igorT reviewed Dec 31, 2014
View reviewed changes

mixonic reviewed Dec 31, 2014
View reviewed changes

rwjblue reviewed Dec 31, 2014
View reviewed changes

lukemelia mentioned this pull request Jan 1, 2015

[DOC] Add inline docs to stream util methods and rename chainStream to chain. #10105

Merged

lukemelia force-pushed the helper-binding-support branch from 6d83dba to 5549f09 Compare January 1, 2015 23:53

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

lukemelia force-pushed the helper-binding-support branch from 5549f09 to a959165 Compare January 2, 2015 00:11

rwjblue reviewed Jan 2, 2015
View reviewed changes

rwjblue added the Needs Team Discussion label Jan 2, 2015

stefanpenner reviewed Jan 2, 2015
View reviewed changes

[FEATURE ember-htmlbars-component-helper] Implement `{{component path…

16b7023

….resolving.to.component.name}}` to dynamically update the component when the bound value changes.

lukemelia force-pushed the helper-binding-support branch from 7b2efd3 to 16b7023 Compare January 2, 2015 20:23

rwjblue removed the Needs Team Discussion label Jan 2, 2015

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

Merge pull request #10093 from lukemelia/helper-binding-support

eff1014

Implement {{component}} helper

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

lukemelia deleted the helper-binding-support branch January 3, 2015 19:43

jasonmit mentioned this pull request Jan 4, 2015

Add deprecation warning once {{component}} PR lands in a stable release minutebase/ember-dynamic-component#4

Closed

taras mentioned this pull request Jan 31, 2015

Ember 2.0 EmberSherpa/ember-helpers-render-component#1

Closed

jbrown mentioned this pull request Feb 10, 2015

Render Helper cannot accept dynamic property as its first parameter, though the Partial Helper can? #9545

Closed

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

using official component helper see: emberjs/ember.js#10093

978d98d

stefanpenner mentioned this pull request Jun 28, 2015

Is there a way to render a component instance? #11573

Closed


		var self = this;

		this._boundViewOptions.viewStream.subscribe(function() {

Implement {{component}} helper #10093

Implement {{component}} helper #10093

Conversation

lukemelia commented Dec 31, 2014

lukemelia commented Dec 31, 2014

Choose a reason for hiding this comment

igorT commented Dec 31, 2014

Choose a reason for hiding this comment

mixonic commented Dec 31, 2014

igorT commented Dec 31, 2014

tomdale commented Dec 31, 2014

Choose a reason for hiding this comment

mixonic commented Dec 31, 2014

Choose a reason for hiding this comment

workmanw commented Dec 31, 2014

igorT commented Dec 31, 2014

mmun commented Dec 31, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukemelia commented Jan 1, 2015

workmanw commented Jan 1, 2015

rwjblue commented Jan 1, 2015

workmanw commented Jan 1, 2015

rwjblue commented Jan 1, 2015

mmun commented Jan 1, 2015

lukemelia commented Jan 2, 2015

Choose a reason for hiding this comment

mmun commented Jan 2, 2015

tomdale commented Jan 2, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobyzerner commented Jan 3, 2015

stefanpenner commented Jan 3, 2015

mmun commented Jan 3, 2015

bcardarella commented Jan 3, 2015

twokul commented Jan 3, 2015

rwjblue commented Jan 3, 2015

lukemelia commented Jan 3, 2015

rwjblue commented Jan 3, 2015

rwjblue commented Jan 11, 2015

shinzui commented Mar 5, 2015

lukemelia commented Mar 5, 2015

shinzui commented Mar 5, 2015

sandstrom commented Jun 16, 2015

ef4 commented Jun 16, 2015

sandstrom commented Jun 16, 2015