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 Modifiers #112

Open
wants to merge 4 commits into
base: master
from

Conversation

Projects
None yet
@mixonic
Member

mixonic commented Jan 24, 2016

Rendered

@rwjblue

This comment has been minimized.

Member

rwjblue commented Jan 24, 2016

This looks good, thanks for taking the time to put it together.

I have a few thoughts/questions:

  • From the examples, I assume that this.attrs is roughly the same as in Ember.Component's today. In "curly components" the mutable object confusion in this.attrs. is very annoying. Why introduce yet another place where this.attrs bizarro land still exists? There also does not seem to be a path forward away from these as two way bound objects, since you wouldn't be able to lean on angle bracket syntax inside of an element. I would prefer to see either no this.attrs or have this.attrs not be two-way bound.
  • Can you detail which hooks fire in server side rendering (aka Fastboot)?
  • We have generally regretted passing attrs, newAttrs, oldAttrs, etc to the various hooks. Should we avoid that mistake here?
  • Will element modifiers only be looked for while inside an element without assignment? Given <button {{foo}} data-bar={{foo}}></button>, element-modifier:foo would be used for the invocation inside the element but helper:foo would be used for the data-bar usage. Is this correct?
  • Similar to above, will these two usages use different foo objects: <button {{foo}}>{{foo}}</button>. One for the element space invocation (element-modifier:foo) and another for the usage in "normal" space (helper:foo)?

Again, great job on this, and thank you for pushing this forward!

@rwjblue

This comment has been minimized.

Member

rwjblue commented Jan 24, 2016

We should also detail how this interacts with angle bracket components.

  • Will <my-button {{add-event-listener 'click' (action 'click')}}></my-button> "work" in some way?
  • Would element space modifiers be usable from both the invoking side and the components own layout?
@chadhietala

This comment has been minimized.

Member

chadhietala commented Jan 26, 2016

I'm wondering if there is value in giving well defined and dependent hooks. My worry is around the higher potential of memory leaks being introduced into a project. If you had hooks like setupListeners and teardownListeners it is clear what you should implement there. Furthermore from a framework point of view, a development version of Ember could do something like:

let hasListener = !!element.setupListeners;
let hasRemoveListener = !!element.teardownListeners;

assert(`You setup an event listener on ${element.toString()}, but did not tear it down in `teardownListeners`. If you are not setting up a listener please use another hook.`, component.setupListeners && !hasRemoveListener)
@mmun

This comment has been minimized.

Member

mmun commented Jan 27, 2016

Totally an aside, but I hope if we ever have a helper like add-event-listener we call it on instead.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Jan 27, 2016

Mixing existing ideas (the global dispatcher) with this approach, this would solve the problem that onclick={{action 'foo'}} makes extremely complicated to use event delegation.

Something like

<ul>
  {{#each option as |opt|}}
    <li {{dispatch-event 'mousedown' (action "choose" opt)}}></li>
  {{/each}}
</ul>

could use Ember's global dispatcher and avoid adding/removing loads of event handlers if the collection changes?

@tchak

This comment has been minimized.

Member

tchak commented Jan 27, 2016

In pre 2.0 lots of confusion was coming from the fact that Ember had way too much "primitives". I was hoping we were on the right path with components only in 2.0. But we introduced state full helpers and now this. This is not a complaint out of nowhere - I already found myself in situations where I had to explain to some people how a state full helper is different from a component... Feels like explaining how a view is different form a component in 1.0... I understand that there is a problem to be solved here, but I feel that this will extend api surface in the area where it is already pretty large.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Jan 27, 2016

To some extent I feel the same way than @tchak about this, and I wonder if there is any way of unify helpers with this. I understand that there is a API gap that has to be filled.

Some sort of helpers with access to the DOM of the element they are invoked in. Perhaps just a stateful helper that happens to have lifecycle hooks other than compute and those hooks are invoked with the DOM element as the first argument.

I don't know if can be considered having less primitives, because it's basically adding more capabilities to one existing primitive.

Something that I like is the fact that this would remove the dicotomy of action is a keyword or a helper depending on the context where it's invoked.

<li onclick={{action "foo"}}></action> <-- is a helper
<li {{action "foo"}}></action> <-- is a keyword

It would formalize an existing magic in ember.

@jgwhite

This comment has been minimized.

jgwhite commented Jan 27, 2016

Thanks @mixonic, this is excellent.

From an addon-author’s point of view, I would love to move the API for ember-sortable towards something like:

{{! my-cool-template.hbs}}
<ul>
  {{#sortable-group as |group|}}
    {{#each items as |item|}}
      <li {{group.sortable-item model=item}}>{{item.name}}</li>
    {{/each}}
  {{/sortable-group}}
</ul>
{{! sortable-group.hbs}}
{{yield (hash
  sortable-item=(element-modifier "sortable-item" group=this)
)}}

Note the assumed availability of yielding contextual-element-modifiers. Did you see that as being a possibility?

During rendering and teardown of a target element, any attached element
modifiers will execute a series of hooks. These hooks are:
* `willInsertElement` (only upon initial render)

This comment has been minimized.

@krisselden

krisselden Jan 29, 2016

Member

what is the use case for this with an element modifier? I was thinking these would be scope to didInsertElement and willDestroyElement

},
didRender() {
document.addEventListener(this.attrs.mouseover, this._logMouseover);

This comment has been minimized.

@krisselden

krisselden Jan 29, 2016

Member

shouldn't this be this.attrs.logEvent?

This comment has been minimized.

@knownasilya

knownasilya Sep 8, 2016

Contributor

I submitted a PR against the rfc too fix this.

@stefanpenner

This comment has been minimized.

Member

stefanpenner commented Jan 29, 2016

Am I correct in assuming these are isomorphic to decorators, maybe a class decorator if one assumes each element is a class (for this discussion)

@krisselden

This comment has been minimized.

Member

krisselden commented Jan 29, 2016

Should this have a this.component in the case it is an Ember component?

@runspired runspired referenced this pull request Feb 2, 2016

Open

Capture Based Eventing #86

@mixonic

This comment has been minimized.

Member

mixonic commented Feb 8, 2016

Thanks for the early feedback all. I'm still digesting but will take a pass at improvements soon.

From @rwjblue:

From the examples, I assume that this.attrs is roughly the same as in Ember.Component's today. In "curly components" the mutable object confusion in this.attrs. is very annoying. Why introduce yet another place where this.attrs bizarro land still exists? There also does not seem to be a path forward away from these as two way bound objects, since you wouldn't be able to lean on angle bracket syntax inside of an element. I would prefer to see either no this.attrs or have this.attrs not be two-way bound.

To me, this is the most concerning comment. The proposal as it stands leans heavily on the API of components, with the goal not introducing a new set of hook names when they already largely match components. Using attrs was intended to keep with that goal, but @rwjblue is right that attrs is not mature enough to consider a starting point. Unfortunately that leaves me going back to the drawing for for wholly new hooks, at least one of which would need the params, hash passed like it is for helpers.

@taras

This comment has been minimized.

taras commented Jun 26, 2016

@mixonic @rwjblue do you have any thoughts on what the story with this RFC is going to be given work that's happening with Glimmer 2 and everything that's going on?

@mixonic

This comment has been minimized.

Member

mixonic commented Jun 27, 2016

@taras I'm still very much in favor of completing an RFC for this feature, though the implementation is indeed very likely blocked on Glimmer 2.

The API design is somewhat intertwined with that of Glimmer components, since I would like the way attrs are passed to match the Glimmer component API. For example this seems plausible:

<div
  class="red button"
  {{on-event 'click' (action 'save' model) bubble=false}}
>Save</div>

Roughly:

  • ember-prop passes to this.args['ember-prop']
  • element modifier gets a this.element
  • if there is a glimmer component, element modifier gets this.component
  • lifecycle hooks match those of Glimmer components

Again some of this is directly related to what APIs we design for Glimmer components, so I'll need to coordinate w/ @wycats and others to get consensus. I'm eager to take another pass :-)

@knownasilya

This comment has been minimized.

Contributor

knownasilya commented Sep 8, 2016

Looking forward to some of this picking up now that Glimmer2 has basically landed.

@cibernox

This comment has been minimized.

Contributor

cibernox commented Sep 12, 2017

This is also something I find interesting and it's a shame it has been staled for a year. Was it abandoned for specific reasons?

@knownasilya

This comment has been minimized.

Contributor

knownasilya commented Sep 12, 2017

Probably the priorities have been diverted, but this seems like an important topic, especially for glimmer.js and it's custom elements story. It should work its way upstream I think.

@mixonic

This comment has been minimized.

Member

mixonic commented Sep 12, 2017

I think progress on this is again tractable. The biggest blocker is the API for how to access positional and named arguments inside the modifier. The hooks should be normalized to match those on Glimmer components.

I'm wary of trying to push it forward before closing up the namespaces/addon support for module unification, re: my own time management 😬

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