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

Element and Fragment RFC #37

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
@ef4
Contributor

ef4 commented Feb 27, 2015

RFC

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

I like this API because it can be implemented under a feature flag pretty cleanly with the presence of <element> opting you in.

Is there a use case for components without templates? I'm thinking no. Things like {{#each}} are handled at the helper level of abstraction. There's a tug-of-war between the in-template APIs and in-class APIs for components. With the tag I agree that we can likely do away with the in-class APIs (classNameBindings, attributeBindings, elementId, tagName, etc).

Since is new API we can disable these in-class APIs. I think it would be a shame not to take that opportunity.

Member

mmun commented Feb 27, 2015

I like this API because it can be implemented under a feature flag pretty cleanly with the presence of <element> opting you in.

Is there a use case for components without templates? I'm thinking no. Things like {{#each}} are handled at the helper level of abstraction. There's a tug-of-war between the in-template APIs and in-class APIs for components. With the tag I agree that we can likely do away with the in-class APIs (classNameBindings, attributeBindings, elementId, tagName, etc).

Since is new API we can disable these in-class APIs. I think it would be a shame not to take that opportunity.

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Feb 27, 2015

Contributor

The main issue with getting rid of classNameBindings and friends is inheritance. For example, we would need a good way to do the kind of things that Ember's TextSupport Mixin does using attributeBindings, without forcing every component that mixes it in to copy all the attributes into their own template's <element>.

There are several possibilities for this. One would be to make attributeBindings be an argument to <element>, so you can say <element attributeBindings=textSupportAttributes>. Another would be a compile-time macro system so you can put <text-support-element> in your template and it would expand to <element autocapitalize={{autocapitalize}} autocorrect={{autocorrect}} ... >.

Contributor

ef4 commented Feb 27, 2015

The main issue with getting rid of classNameBindings and friends is inheritance. For example, we would need a good way to do the kind of things that Ember's TextSupport Mixin does using attributeBindings, without forcing every component that mixes it in to copy all the attributes into their own template's <element>.

There are several possibilities for this. One would be to make attributeBindings be an argument to <element>, so you can say <element attributeBindings=textSupportAttributes>. Another would be a compile-time macro system so you can put <text-support-element> in your template and it would expand to <element autocapitalize={{autocapitalize}} autocorrect={{autocorrect}} ... >.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

Alternatively, if each component embeds it's template (rather than being looked up separately, which adds complications because it requires a name) AND templates exposed metadata related to element, we could implement "template" inheritance.

Member

mmun commented Feb 27, 2015

Alternatively, if each component embeds it's template (rather than being looked up separately, which adds complications because it requires a name) AND templates exposed metadata related to element, we could implement "template" inheritance.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

I like the potential of

export Ember.Component.extend(TextSupport, {
  template: hbs`<element class="ember-text-field {{class}}" tagName="input">`
})
Member

mmun commented Feb 27, 2015

I like the potential of

export Ember.Component.extend(TextSupport, {
  template: hbs`<element class="ember-text-field {{class}}" tagName="input">`
})
@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Feb 27, 2015

Contributor

I don't quite see how that's different than just keeping attributeBindings. What part of TextSupport are we inheriting? It's template? If so, how does that fit with our template?

Contributor

ef4 commented Feb 27, 2015

I don't quite see how that's different than just keeping attributeBindings. What part of TextSupport are we inheriting? It's template? If so, how does that fit with our template?

This RFC addresses two key problems:
- Ember has no clear concept of a "tagless component". We need such a
concept now that we're moving to a component-centric world.

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

can we explain why this is needed?

@stefanpenner

stefanpenner Feb 27, 2015

Member

can we explain why this is needed?

This comment has been minimized.

@rwjblue

rwjblue Feb 27, 2015

Member

@stefanpenner - We need this because we use this concept internally. There are cases where you might want to implement things like our internal {{each}} and {{if}} helpers as components, and you wouldn't need/want there to have to be an extra element.

@rwjblue

rwjblue Feb 27, 2015

Member

@stefanpenner - We need this because we use this concept internally. There are cases where you might want to implement things like our internal {{each}} and {{if}} helpers as components, and you wouldn't need/want there to have to be an extra element.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

I don't see the point of attribute bindings at all, at least in the one-way case. We shouldn't need to specify that something is bound to a DOM attribute - it should always happen. {{input placeholder=foo}} should "just work" without having to specify 'placeholder' somewhere.

The mutable attribute case should be fairly rare. For example, updating a class name of a component based on its current state. I believe react completely punts on this (you'd have to manually mutate the DOM). We support it already with classNameBindings and we could make it work if we needed to, but it doesn't seem critical.

I just think of TextSupport as mixing in event handlers to fire actions.

Member

mmun commented Feb 27, 2015

I don't see the point of attribute bindings at all, at least in the one-way case. We shouldn't need to specify that something is bound to a DOM attribute - it should always happen. {{input placeholder=foo}} should "just work" without having to specify 'placeholder' somewhere.

The mutable attribute case should be fairly rare. For example, updating a class name of a component based on its current state. I believe react completely punts on this (you'd have to manually mutate the DOM). We support it already with classNameBindings and we could make it work if we needed to, but it doesn't seem critical.

I just think of TextSupport as mixing in event handlers to fire actions.

@wagenet

This comment has been minimized.

Show comment
Hide comment
Member

wagenet commented Feb 27, 2015

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

Nowhere am I suggesting that the behaviour not be inheritable. I am saying one way attribute bindings should work without explicitly listing them.

Member

mmun commented Feb 27, 2015

Nowhere am I suggesting that the behaviour not be inheritable. I am saying one way attribute bindings should work without explicitly listing them.

@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 27, 2015

Member

@mmun but how do you compose that? Are you suggesting a way in which templates can be combined?

Member

wagenet commented Feb 27, 2015

@mmun but how do you compose that? Are you suggesting a way in which templates can be combined?

template to:
````handlebars
<element class="special {{class}}" data-my-id={{id}} tagName="span">

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

is tagName bindable? Is i still be set from the JS side, or always form the template side.

@stefanpenner

stefanpenner Feb 27, 2015

Member

is tagName bindable? Is i still be set from the JS side, or always form the template side.

This comment has been minimized.

@rwjblue

rwjblue Feb 27, 2015

Member

It should not be.

@rwjblue

rwjblue Feb 27, 2015

Member

It should not be.

for `<element>` to be the mandatory one, since `<element>` does useful
work and represents a real DOM element.
`<fragment>` is only needed due to upgrade-compatibility

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

should we propose a timeline for this, or is it pre-mature.

@stefanpenner

stefanpenner Feb 27, 2015

Member

should we propose a timeline for this, or is it pre-mature.

not and automatically make it by a Component or Fragment. This idea
was rejected because it's a footgun -- for example, accidentally
changing your template from one kind to the other would change the
sematnics of your `$` method.

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

typo: sematnics -> semantics

@stefanpenner

stefanpenner Feb 27, 2015

Member

typo: sematnics -> semantics

# Unresolved questions
- Should `Ember.Component` extend `Ember.Fragment`?

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

yes, I believe so.

@stefanpenner

stefanpenner Feb 27, 2015

Member

yes, I believe so.

- Should `Ember.Component` extend `Ember.Fragment`?
- Should `Ember.Fragment` have a `$` method that matches its DOM

This comment has been minimized.

@stefanpenner

stefanpenner Feb 27, 2015

Member

as its dom range is dynamic, I don't believe so. Imagine the following scenario.

<fragment>
  <h1>asdf</h1>
  {{#if theSkyIsFalling}}
    <p>Run!!</p>
  {{/if}}
</fragment>

this.$() can easily become stale.

@stefanpenner

stefanpenner Feb 27, 2015

Member

as its dom range is dynamic, I don't believe so. Imagine the following scenario.

<fragment>
  <h1>asdf</h1>
  {{#if theSkyIsFalling}}
    <p>Run!!</p>
  {{/if}}
</fragment>

this.$() can easily become stale.

@stefanpenner

This comment has been minimized.

Show comment
Hide comment
@stefanpenner

stefanpenner Feb 27, 2015

Member

further clarification:

both <element></element> and <fragment></fragment> must be top level, with no siblings.

For example, the following scenarios would raise an exception – likely at parse time.

  • non element or fragment root, with fragment or element descendent
<h1> 
  <element> ...</element>
</h1>
  • multiple root siblings
<h1> something </h1>
<element> ...</element>
  • element with a descended fragment (and obviously, fragment with a descended element)
<element>
  <fragment> </fragment>
</element>
<fragment>
  <element> </element>
</fragment>
Member

stefanpenner commented Feb 27, 2015

further clarification:

both <element></element> and <fragment></fragment> must be top level, with no siblings.

For example, the following scenarios would raise an exception – likely at parse time.

  • non element or fragment root, with fragment or element descendent
<h1> 
  <element> ...</element>
</h1>
  • multiple root siblings
<h1> something </h1>
<element> ...</element>
  • element with a descended fragment (and obviously, fragment with a descended element)
<element>
  <fragment> </fragment>
</element>
<fragment>
  <element> </element>
</fragment>
@wagenet

This comment has been minimized.

Show comment
Hide comment
@wagenet

wagenet Feb 27, 2015

Member

As it turns out, every attribute binding declared in TextSupport could just be explicit as part of the <element>. To make the issue more clear, here's an example I put together: https://gist.github.com/wagenet/989b759848e8ee8043a8. If it wasn't for inheritance, I'd be happy to ditch the whole notion of attribute bindings. However, without some sort of equivalent we do run into the problem I demonstrate.

Member

wagenet commented Feb 27, 2015

As it turns out, every attribute binding declared in TextSupport could just be explicit as part of the <element>. To make the issue more clear, here's an example I put together: https://gist.github.com/wagenet/989b759848e8ee8043a8. If it wasn't for inheritance, I'd be happy to ditch the whole notion of attribute bindings. However, without some sort of equivalent we do run into the problem I demonstrate.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Feb 27, 2015

Member

Yeah, that's a good example to keep in mind!

Member

mmun commented Feb 27, 2015

Yeah, that's a good example to keep in mind!

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus Feb 28, 2015

I really like the first part of the proposal (ie. Ember.Fragment and Ember.Component distinction), but I'm not so sure about explicit definition of <element> or <fragment>. I agree that having 2 APIs for the same thing may be confusing, but as other commenters noted, we will still be left with 2 APIs.

A possible solution would be to allow to "merge" inherited attributes in a template, although I'm not sure if it's technically doable, because I don't know HTMLBars too well. It could look like:

var FooComponent = Ember.Component.extend(),
    BarComponent = FooComponent.extend();
{{! templates/components/foo-component.hbs }}
<element class="foo">Something</element>
{{! templates/components/bar-component.hbs }}
<element class="bar" {{super-attributes}}>{{super-content}}</element>

Using {{bar-component}} would render <div class="foo bar">Something</div>

drogus commented Feb 28, 2015

I really like the first part of the proposal (ie. Ember.Fragment and Ember.Component distinction), but I'm not so sure about explicit definition of <element> or <fragment>. I agree that having 2 APIs for the same thing may be confusing, but as other commenters noted, we will still be left with 2 APIs.

A possible solution would be to allow to "merge" inherited attributes in a template, although I'm not sure if it's technically doable, because I don't know HTMLBars too well. It could look like:

var FooComponent = Ember.Component.extend(),
    BarComponent = FooComponent.extend();
{{! templates/components/foo-component.hbs }}
<element class="foo">Something</element>
{{! templates/components/bar-component.hbs }}
<element class="bar" {{super-attributes}}>{{super-content}}</element>

Using {{bar-component}} would render <div class="foo bar">Something</div>

@ef4

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Feb 28, 2015

Contributor

I incorporated feedback on how to handle HTML attributes in a way that lets us deprecate attributeBindings and friends, while still handling inheritance.

Contributor

ef4 commented Feb 28, 2015

I incorporated feedback on how to handle HTML attributes in a way that lets us deprecate attributeBindings and friends, while still handling inheritance.

@jerel

This comment has been minimized.

Show comment
Hide comment
@jerel

jerel Feb 28, 2015

At first I disliked the idea of <element> and <fragment> in templates but the more I ponder it the more I think that having the element defined in the template instead of in JS makes components easier to reason about. I'm 👍

There's another use case that I think <fragment> can solve nicely: integrating third party libraries with the Ember component structure and actions. I've been doing this in my own apps but the current incarnation of components doesn't make it ideal (even with tagName set to '' a component still goes through the render steps and outputs a textNode) and performance is OK but not great. Future example:

A map component template that doesn't create any HTML itself:

<fragment>
    {{#each location in locations}}
        {{#if location.onMap}}
            {{marker latLng=location map=map}}
        {{/if}}
    {{/each}}
</fragment>

And the marker component:

export default Ember.Component.extend({
    click: function() {
        this.sendAction('openPopup', this.marker);
    },
    render: function() {
        var map = this.get('map'),
            latLng = this.get('latLng');
        this.marker = this.map.addMarker([latLng[0], latLng[1]]).addTo(map);
        this.marker.on('click', this.click());
    },
    destroy: function() {
        this.marker.off('click');
        this.get('map').removeMarker(this.marker);
    },
});

Hope this isn't off-topic for this RFC but figured I'd add it so you can see how it might be used/abused.

jerel commented Feb 28, 2015

At first I disliked the idea of <element> and <fragment> in templates but the more I ponder it the more I think that having the element defined in the template instead of in JS makes components easier to reason about. I'm 👍

There's another use case that I think <fragment> can solve nicely: integrating third party libraries with the Ember component structure and actions. I've been doing this in my own apps but the current incarnation of components doesn't make it ideal (even with tagName set to '' a component still goes through the render steps and outputs a textNode) and performance is OK but not great. Future example:

A map component template that doesn't create any HTML itself:

<fragment>
    {{#each location in locations}}
        {{#if location.onMap}}
            {{marker latLng=location map=map}}
        {{/if}}
    {{/each}}
</fragment>

And the marker component:

export default Ember.Component.extend({
    click: function() {
        this.sendAction('openPopup', this.marker);
    },
    render: function() {
        var map = this.get('map'),
            latLng = this.get('latLng');
        this.marker = this.map.addMarker([latLng[0], latLng[1]]).addTo(map);
        this.marker.on('click', this.click());
    },
    destroy: function() {
        this.marker.off('click');
        this.get('map').removeMarker(this.marker);
    },
});

Hope this isn't off-topic for this RFC but figured I'd add it so you can see how it might be used/abused.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Mar 1, 2015

The strategy for accessing your "parent template" needs to be explained. I like embedding the template into the class, but we could consider using module reflection to infer the name of the parent component's template.

The strategy for accessing your "parent template" needs to be explained. I like embedding the template into the class, but we could consider using module reflection to infer the name of the parent component's template.

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Mar 1, 2015

Owner

I think the implementation detail of how we find the parent class's template is relatively straightforward. Embedding should always be optional -- it's a good choice for a library. Otherwise we follow naming conventions.

Owner

ef4 replied Mar 1, 2015

I think the implementation detail of how we find the parent class's template is relatively straightforward. Embedding should always be optional -- it's a good choice for a library. Otherwise we follow naming conventions.

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Mar 1, 2015

You can't recover the name of a component simply from it's class, hence you cannot recover a component's container-registered template from its class either. Give a string like "x-foo" you can, of course, look up both the template and the class, which is what we do.

Now say you have XFooComponent which extends XParentComponent. Your template will contain the string {{x-foo}} but is it knows about templates/x-foo, but how will it know about templates/x-parent?

This problem is compounded by namespaced components like {{foo/x-bar}}.

mmun replied Mar 1, 2015

You can't recover the name of a component simply from it's class, hence you cannot recover a component's container-registered template from its class either. Give a string like "x-foo" you can, of course, look up both the template and the class, which is what we do.

Now say you have XFooComponent which extends XParentComponent. Your template will contain the string {{x-foo}} but is it knows about templates/x-foo, but how will it know about templates/x-parent?

This problem is compounded by namespaced components like {{foo/x-bar}}.

@mmun

This comment has been minimized.

Show comment
Hide comment
@mmun

mmun Mar 1, 2015

This doesn't cover inheritance (e.g. mergeable class name bindings across classes). It would be nice to crack this nut.

This doesn't cover inheritance (e.g. mergeable class name bindings across classes). It would be nice to crack this nut.

This comment has been minimized.

Show comment
Hide comment
@ef4

ef4 Mar 1, 2015

Owner

It covers most of inheritance, but you're right, the case of merging a single attribute is still missing. I think we can do that too.

What's needed is the equivalent of super for accessing your parent's value for an attribute. class="{{super}} additional".

Owner

ef4 replied Mar 1, 2015

It covers most of inheritance, but you're right, the case of merging a single attribute is still missing. I think we can do that too.

What's needed is the equivalent of super for accessing your parent's value for an attribute. class="{{super}} additional".

@drogus

This comment has been minimized.

Show comment
Hide comment
@drogus

drogus Mar 1, 2015

@ef4 do you think that ability to use parent's template content would be beneficial? An API for overriding parent's attributes looks good, but I don't see anything for content. I'm not sure how important that is, but it was one of the things that I remember using, for example to add an attribute to a component, but still leave the parent's template.

Also, it's still missing how could mixins be used without attributeBindings. Here, too, I'm not sure how important that is, but probably without covering this case `attributeBindings API shouldn't be deprecated.

drogus commented Mar 1, 2015

@ef4 do you think that ability to use parent's template content would be beneficial? An API for overriding parent's attributes looks good, but I don't see anything for content. I'm not sure how important that is, but it was one of the things that I remember using, for example to add an attribute to a component, but still leave the parent's template.

Also, it's still missing how could mixins be used without attributeBindings. Here, too, I'm not sure how important that is, but probably without covering this case `attributeBindings API shouldn't be deprecated.

@mixonic

This comment has been minimized.

Show comment
Hide comment
@mixonic

mixonic Mar 14, 2015

Member

Great! It took some thinking, but I understand why the both the two classes and two markups are needed to manage backwards compat. Very good.

top-level element tag

Stef says:

both and must be top level, with no siblings.

This follows how component layouts work today. Seems correct in all ways.

Member

mixonic commented Mar 14, 2015

Great! It took some thinking, but I understand why the both the two classes and two markups are needed to manage backwards compat. Very good.

top-level element tag

Stef says:

both and must be top level, with no siblings.

This follows how component layouts work today. Seems correct in all ways.

@ernesto-jimenez

This comment has been minimized.

Show comment
Hide comment
@ernesto-jimenez

ernesto-jimenez Apr 2, 2015

This RFC came to mind while watching the video from @ebryn's minitalk in EmberConf about ember-component-css.

The add-on allows you to drop component-specific CSS in a styles.css in a pod along with template.hbs and component.js, then it automatically adds a namespace class to the component's root tag and namespaces all the CSS.

In order to style the component's root tag it uses this syntax:

& {
  padding: 2px;
}

If this RFC gets implemented, & could be changed to element like this:

element {
  padding: 2px;
}

I'm just commenting it here because it seems the RFC introduces new conventions that would help namespacing component's CSS in the future, which is a problem apps will run into as more components get shared between apps.

This RFC came to mind while watching the video from @ebryn's minitalk in EmberConf about ember-component-css.

The add-on allows you to drop component-specific CSS in a styles.css in a pod along with template.hbs and component.js, then it automatically adds a namespace class to the component's root tag and namespaces all the CSS.

In order to style the component's root tag it uses this syntax:

& {
  padding: 2px;
}

If this RFC gets implemented, & could be changed to element like this:

element {
  padding: 2px;
}

I'm just commenting it here because it seems the RFC introduces new conventions that would help namespacing component's CSS in the future, which is a problem apps will run into as more components get shared between apps.

@ebryn

This comment has been minimized.

Show comment
Hide comment
@ebryn

ebryn Apr 3, 2015

Member

@ernesto-jimenez Yes, my ember-component-css project is built with routable components/pods in mind!

I"m going to be working on the implementation of routable components. I've actually got some hacks available for use right now in an addon: https://github.com/ebryn/ember-routable-components

Member

ebryn commented Apr 3, 2015

@ernesto-jimenez Yes, my ember-component-css project is built with routable components/pods in mind!

I"m going to be working on the implementation of routable components. I've actually got some hacks available for use right now in an addon: https://github.com/ebryn/ember-routable-components

@tomdale

This comment has been minimized.

Show comment
Hide comment
@tomdale

tomdale Jun 7, 2015

Member

Superseded by #60 (sorry @ef4)

Member

tomdale commented Jun 7, 2015

Superseded by #60 (sorry @ef4)

@tomdale tomdale closed this Jun 7, 2015

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