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

Attribute Actions #100

Closed
wants to merge 4 commits into from
Closed

Attribute Actions #100

wants to merge 4 commits into from

Conversation

mixonic
Copy link
Sponsor Member

@mixonic mixonic commented Oct 18, 2015

Rendered

If I have missed a use-case of classic actions, please help me out by describing it!

@mmun
Copy link
Member

mmun commented Oct 18, 2015

Can't we keep <button onclick={{foo}}> as the syntax but change the implementation? (i.e. don't buttonInstance.onclick = foo, let ember do the subscription instead as you would have with kebab actions)

@ef4
Copy link
Contributor

ef4 commented Oct 18, 2015

We either need to do what @mmun suggests (make onclick a perfect synonym for on-click) or we need to loudly explode if somebody tries onclick). Otherwise we leave a nasty landmine where forgetting a single dash (which is easy to forget, given native DOM naming) silently causes different behavior.

Kebab actions will be invoked with the raw DOM event (not the jQuery event)...

+1 for that. This eliminates one of the places where jQuery leaks into Ember's public API, making it hard to safely support alternatives or a jQuery-optional world.

@rwjblue
Copy link
Member

rwjblue commented Oct 18, 2015

I think making the non-dashed versions break would be a mistake (there were a number of core team members suggesting "just use the DOM API's, its wonderful" for 1.13+). We could try to map the most common ones automatically with a deprecation/warning, but I think we need to continue to allow them to work...

@ef4
Copy link
Contributor

ef4 commented Oct 18, 2015

That's why I would favor upgrading them automatically, rather than disallowing them.

What would be the semantic differences if we started treating onclick as on-click?

@rwjblue
Copy link
Member

rwjblue commented Oct 18, 2015

I believe the main difference is the way they would bubble (changing to on-click means it would fire after any parent elements and today it fires first), but I'm not super concerned with that. I think upgrading with a warning/deprecation would be fine.


```hbs
{{! app/templates/components/my-input.hbs }}
<input on-change={{action 'log' value='target.value'}} />
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to deprecate value in favor of valuePath (which is what it actually is), but maybe that is a discussion for another issue/PR.

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.

Yeah I think that is uncontroversial, just needs to happen. I suppose it should happen with these changes just for ergonomics, since this is where valuePath= should become more commonly used.

@rwjblue
Copy link
Member

rwjblue commented Oct 18, 2015

👍 from me

@stefanpenner
Copy link
Member

some quick thoughts as i read:

  • Do we plan to migrate away from them entirely? if so when?
  • Do we care about onClick={{.. }}andon-click={{...}} and onClick="{{}}" and onclick={{}} potentially existing and behaving differently?
  • on-click="{{ }}" is quoted form a failure scenario?
  • since these aim to use the even dispatcher instead of a property set of the handler, is there interopt thoughts on the future of webcomponents

@alexspeller
Copy link
Contributor

One thing I'd like to try and maintain is action bubbling up the active route tree. The router as a state machine being able to respond to actions differently in different states is hugely useful and powerful, so I'd like to call out here making sure that this is still possible however actions are invoked in the future (I've had some discussion with @rwjblue about this on slack not long ago).

@rwjblue
Copy link
Member

rwjblue commented Oct 18, 2015

One thing I'd like to try and maintain is action bubbling up the active route tree.

This doesn't directly impact that, though it does reduce the exposure of route actions directly from a template. I personally think that is a good thing, and a collaborating service (ala the routing service RFC) can be used as the entry point into the router's hierarchical action structure.

In other words, I think that it is absolutely important to ensure that the router still has the ability to handle actions (and this does not change that functionality). However, I do not think that targeting the router directly from a template is a good thing (and this functionality will almost certainly be removed when we move to routable components and away from controllers).

@mixonic
Copy link
Sponsor Member Author

mixonic commented Oct 22, 2015

@stefanpenner some reactions:

Do we plan to migrate away from them entirely? if so when?

From classic actions? This RFC does not suggest their immediate deprecation or removal. I would prefer to migrate the documentation and community to the new style of usage before adding the deprecation. My ideal timeline would be kebabs in 2.x, deprecation of classic in 2.x+1. Will update to make this recommendation explicit.

Do we care about onClick={{.. }}andon-click={{...}} and onClick="{{}}" and onclick={{}} potentially existing and behaving differently?

I think this is being raised elsewhere by @mmun and @ef4 as well. Needs some thought. I was not as concerned about it as y'all seem to be initially.

on-click="{{ }}" is quoted form a failure scenario?

Will call out the fact that any non-function should throw via an assert.

since these aim to use the even dispatcher instead of a property set of the handler, is there interopt thoughts on the future of webcomponents

:-( I'm not too excited about holding this work on a clear plan for webcomponents, so I would say no. We don't explicitly test or support web components for any other Ember features. IMO, webcomponent support and the changes that would happy to ensure it is a whole different RFC.

Open to feedback. Perhaps there is something conservative, like attaching the event dispatching and additionally setting the property of on-foo to equal the action function? What is "conventional" for attaching callbacks to a webcomponent?

@alexspeller I believe the architectural concerns you've raised are valid and shared in some form by core- But I also think they are more related to routable components than to this proposal. The trick of routable components is how we pass actions down to the component as attrs, and that needs to be RFC'd in the next routable components plan (once we get there).


Kebab actions are lazy. If a kebab action of `on-flummux` is used, then Ember
should listen for the event of `flummux` on the root element and dispatch
that action when it fires.
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am reading correctly, this means that in a child component you would sendAction('flummux') (or is it going to be just send everywhere with the new API?).

I think this would be a welcome change in my book. To replicate as close to native browser look and feel, I currently use onSubmit to listen for form components to mop up all the user input I need, but this is a bit awkward since it means writing sendAction('onSubmit') from my form component. Twiddle Example

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.

@rtablada I don't think this proposes anything different as far as you gist goes. "Ember should listen for the event" and in general "Event Management" applies to regular DOM elements, not components. I'll clarify this in the RFC.

You would continue to use sendAction in a component. Eventually you will just be calling this.attrs['on-click'](), however we don't teach that right now since it means using attrs which are confusing on curly components (auto-mut).

You raise an interesting though regarding glimmer components though. In a glimmer component, given this proposal:

<my-input class="error" on-change={{action 'refresh'}} />
// app/components/my-input.js
export default Ember.GlimmerComponent.extend();
{{! app/templates/components/my-input.hbs }}
<input class="blue" on-mouse-move={{action 'click'}} />

The class attribute is considered merged with the class attribute of the component's root element (making it "blue error"). However this application of an attr to the root element does not apply to the on-change attribute. Instead that is treated as an argument to the object, and attaching event listeners is deemed something only under the control of the component.

I think the above is good and correct. A component invocation should not be able to add event listeners to the root element of the invoked component. I'll update the RFC to discuss this interaction with Glimmer components more closely.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mixonic good to know about just calling this.attrs['on-click'].

Just a thought, maybe a fireOn function in components that is a bit like:

fireOn(name, ...args) {
  const eventName = `on-${name.dasherize()}`;

  if (this.attrs[eventName]) {
    this.attrs[eventName](...args);
  }
}

@samselikoff
Copy link
Contributor

I just watched Matt introduce this at Ember NYC, and it looks awesome. The one comment I had as a first-time observer of the api was, the value part of {{action 'log' value='target.value'}} is a bit confusing.

Matt made the point that

<div {{action 'save' on='mouseOver'}}>Save</div>

is a somewhat awkward api, since it has no equivalent in HTML. But it seems that the value part of

<div on-mouse-over={{action 'log' value='target.value'}}>Save</div>

suffers from the same problem. I understand adding this api would save users from needing to define a new action under the actions hash in the case of using mut

<input on-change={{action (mut firstName) value='target.value'}} />

but it seems we could solve the same problem by using nested s-expressions:

<input on-change={{action (compose (mut firstName) (take 'target.value') ) }} />

If we can do this with existing primitives, I think there'd need to be a really compelling reason to introduce the new api. Also, while the functional composition approach might be slightly harder to explain to a newbie (and I'm not sure this is true), at least they're learning a general programming technique that they can use with their own helpers, rather than another Ember-specific api.

@rtablada
Copy link
Contributor

@samselikoff could be off on this, but value on actions is already in ember and not part of this RFC. Not saying that a take helper is a bad idea but not sure if changing the semantics of value would be part of this RFC?

@samselikoff
Copy link
Contributor

Ah, was unaware. In that case of course I agree we shouldn't change semantics. However I would like to see a unified message around using (or not) using that api.

@rtablada
Copy link
Contributor

Yup, here was the example I remember using it from. http://balinterdi.com/2015/08/29/how-to-do-a-select-dropdown-in-ember-20.html


Sent from Mailbox

On Fri, Oct 23, 2015 at 6:45 AM, Sam Selikoff [email protected]
wrote:

Ah, was unaware. In that case of course I agree we shouldn't change semantics. However I would like to see a unified message around using (or not) using that api.

Reply to this email directly or view it on GitHub:
#100 (comment)

@workmanw
Copy link
Contributor

Kebab actions will be invoked with the raw DOM event (not the jQuery event)...

ZOMG! Thank you! The biggest problem we have with the "classic actions" is there was no access to the events. We often position popovers at the site of the click. Since "classic actions" didn't provide the event, we frequently have to install manual click handlers.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Oct 23, 2015

@bantic and I were brainstorming about some of the outstanding issues here yesterday. This is an adjustment to the RFC, and I'll make the changes later today.

On not choosing the onclick syntax

onclick is a property on DOM nodes with specific behavior, and also an attribute with behavior. I think is it wrong to hijack and change that behavior. Kebabs (on-) allow us to introduce our own semantics based on event delegation, and to not implement other semantics (like setting a string of code to be eval'd).

If we are adding on-, then it does raise the issue of if direction event listener attachment (onclick=) is confusing. I'm not sure it is, but...

A liberal option for dealing with onclick would be to disallow its use entirely. Throw or assert anytime it (or another event listener prop/attr) is used. The most conservative option would be to allow devs to continue using it as they do today. A middle ground is to warn when it is used, nudging people toward using the on- syntax which is almost certainly what they want to be using.

On choosing the on-mouseover

Currently this proposes on-mouse-over as the syntax for kebabs. This is motivated by the fact that event observers in Ember components and the current on= syntax for classic actions are camel case, for example mouseOver.

In a glimmer component world, I don't expect event observers in the JavaScript part of the component to be as common. Instead, they would be attached to the root element in the template. For example:

<my-button on-click={{action 'save'}} />
// app/components/my-button.js
export default Ember.GlimmerComponent.extend({
});
{{! app/templates/components/my-button.hbs }}
<button on-click={{attrs.on-click}}>Save</button>

Given that classic actions are also not a recommended path, we will have removed the two public APIs motivating us to use camel case and dasherized multi-word events.

Thus, instead, I suggest the on- prefix followed by the native event name. on-mouseover, on-click, on-mousedown, etc. No multi-world version. There will be a transition period, but as long as the assumption that event listeners will not be added via JS in glimmer components is true, this seems a better path for the future.

@alexspeller
Copy link
Contributor

If we are adding on-, then it does raise the issue of if direction event listener attachment (onclick=) is confusing. I'm not sure it is, but...

I can absolutely guarantee that if <button on-click={{something}}> and <button onclick={{something}}> have slightly different behaviour, it will become the number one question I answer on slack 😉

I think asserting / warning is definitely the best option here.

@mmun
Copy link
Member

mmun commented Oct 23, 2015

I disagree that is wrong to "hijack" on*. It is not hijacking anymore than value={{foo}} is hijacking an input... There is no semantic for databinding in HTML so we are free to make onfoo={{bar}} do whatever we want.

Regardless, I am ok with on-*. We can't assert because it is de facto part of semver now. A warning is good. We can do it at template compile time and give a good message.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Oct 23, 2015

I disagree that is wrong to hijack on* but OK to hijack value on input...

Is it suggested that we hijack value= on <input>? Where?

@mmun
Copy link
Member

mmun commented Oct 23, 2015

{{input value=foo}}. I edited my comment to explain further.

@rtablada
Copy link
Contributor

@mixonic while I do think that on-* is the way to go, I'm thinking that mouseover is closer to spec and if we shift to one word (to get closer to both web standard AND jquery) kebabs would be the time to do it. Worst case is recommend single word and then alias on-mouse-over.

@wycats
Copy link
Member

wycats commented Oct 23, 2015

Can someone explain to me what change in semantics we're talking about by choosing onclick=?

@mixonic
Copy link
Sponsor Member Author

mixonic commented Oct 23, 2015

Because events managed by Ember are delegated from a listener on the body, there are slight changes in ordering if you attach handlers on your own. This JSBin demonstrates using action and onclick, and you can see that the order of the wrapper click event and the action/onclick attached function are not consistent.

@cibernox
Copy link
Contributor

About the discussion about event delegation, I'm not convinced that discouraging the dashless version is a good idea.

I guess that with onclick there is not much problem, but using event delegation with events that fire all the time, like mouseover is generally bad for performance, because you are going to evaluate every single mouseover event in the entire dom structure to check if target matches one of the delegators.

Probably in that case using the onmouseover is a better solution.

I'm sure that there might be other examples where the non-delegated version has some advantage (predictable callback invocation perhaps?)

@mixonic
Copy link
Sponsor Member Author

mixonic commented Oct 26, 2015

I have further thoughts to add, raised in discussion at the last core meeting. Will add shortly!

@wycats
Copy link
Member

wycats commented Nov 10, 2015

For what it's worth, we've discussed this at great length in core team meetings, and we're converging on a consensus to go with onclick={{func}}.

The EventDispatcher shines when the process of hooking up an event can be subsumed by something else that we're already doing (like creating the DOM node for a component). This means we can create a whole bunch of components and not worry at all about wiring up listeners. That tradeoff isn't true in the totally general case, which is what this discussion has been about.

It is probably worth revisiting how we do events for Ember 3.0, but we're not going to change how events are delegated to components in the middle of the 2.x cycle, and onclick={{action 'foo'}} works because we would have to do special work to make it not work. That's a good sign in my opinion.

@runspired
Copy link
Contributor

I think the argument I'd like to make it that it should be possible for an addon to completely swap out the eventing system in play without extensive hacks into Ember itself.

What made jQuery eventing so powerful was that it wasn't just about fixing cross-browser issues.

jQuery

  • normalized handler methods
  • normalized event objects
  • "normalized" some events by making events bubble that otherwise shouldn't
  • "normalized" the event model in use (bubble)
  • and made it easy to listen on root elements

But jQuery also fast paths handler resolutions attached with jQuery.on for events you trigger with jQuery.trigger, and does so by not triggering a native event until after "fastpath" events have taken place. This means that using jQuery.on for eventing introduces an extra layer of complexity.

Which brings me to my main point: the state of eventing in the Ember ecosystem has hit a point I think approaches a deadly architecture flaw. This rundown is completely ignoring the fact that EventManager is or ever was a thing (which itself adds several more layers to this puzzle).

Eventing in Ember

  • we attach handlers via onclick and similar
  • we attach jQuery handlers of our own in didInsertElement
  • we attach native bubble handlers of our own in didInsertElement
  • we attach native capture handlers of our own in didInsertElement
  • we use event and action methods on components
    • which are both bound using jQuery

If you consider bubbling capture jQuery and on-element to be four different "models of eventing" (which they are, they can interplay but walk a fine line doing so), then that means we have:

  • 4 event models in use
  • 6 very different paths into these 4 event models

Most good devs I talk to have no idea the difference between capture bubble jquery and on-element eventing. There's a rough understanding that attaching listeners to root elements can help performance in some situations, and not much more.

Here's a demo of just how / when all these different methods would trigger.
https://ember-twiddle.com/847601d7383bc60052b4?numColumns=0

The current thinking is that "CSS Scope Invasion" is wrong. I'd argue that something pretty similar is going on in eventing right now. Handlers can signup to manage events from too many directions, and in too many ways, leading to too many conflicts. But because we've all come to rely on jQuery for so long, we've been more blind to this issue than we ought to have been.

I strongly feel that Ember should either opt to manage eventing comprehensively, or opt to not manage it all, but instead open the door for addons to easily create comprehensive solutions of their own.

What exactly this means for onclick vs. on-click in this current RFC, I don't really know. But eventing is broken, and we need to be careful not to box ourselves into a mistake we can't undo until a major bump.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Nov 10, 2015

@runspired It would be very helpful to see a rundown of event object and bubbling inconsistencies in IE9+. Do you have any resources that would make a good read?

@runspired
Copy link
Contributor

@mixonic quite honestly, the best place is the source code for jQuery's eventing.

@runspired
Copy link
Contributor

@mixonic but I can make it a priority tomorrow to generate a comprehensive rundown from that code for easy consumption by the core team.

@krisselden
Copy link

@wycats in order to have something in 3.0 that is different I think work needs to be done to start migrating away from the jquery event dispatcher, much like we had the globals/module resolver. We will support 2 event dispatchers for a while.

@mixonic
Copy link
Sponsor Member Author

mixonic commented Nov 27, 2015

I've looked through

And haven't identified any IE9 issue that would block us from using native event APIs. The quirkiest thing seems to be focusin and focusout events, which are largely polyfilled for old browsers. The shape of the event object has some quirks across browsers but only for old versions of evergreen browsers or old versions of Cordova.

@mixonic mixonic changed the title Kebab Actions Attribute Actions Nov 27, 2015
@mixonic
Copy link
Sponsor Member Author

mixonic commented Nov 27, 2015

I've pushed a significant number of edits to this RFC, and I encourage people to read it again. The introduction spells out the main direction pretty directly.

  • No longer suggests on-click, but onclick
  • No longer suggests using Ember's event manager, but instead attempts to draw the path away from it

@mixonic
Copy link
Sponsor Member Author

mixonic commented Nov 29, 2015

And another small update:

  • Drop any suggestions about click() { style listeners using or not using the event manager.
  • Clarify that addEventListener should be used for attaching listeners. This means web components will need to dispatchEvent to fire listeners, and will not have the function passed as an attribute. How we should work with web components is largely speculative, however there seems to be some consensus around using events for external communication and on* attributes are not treated as handlers for events on web components. Thus adding a listener is preferred. /cc @stefanpenner who I've been back and forth on about this a bit.

official API.
* There are some very small code changes, but largely this is a
messaging and documentation change.
* These "attribute events" should use `element.addEventListener(eventName, handler, false)`
Copy link
Member

Choose a reason for hiding this comment

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

Why?

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.

I will update and attempt to make this clearer.

  • Not all events have accessors. For example onfocusin= does not exist as a property (see #12676), thus it falls back to an attribute. See also discussion at https://github.com/emberjs/rfcs/pull/100/files#r46105178
  • Setting the accessor leaves us with a poor merge strategy for glimmer component root elements. The attached onclick= from the invocation site would stop an internal attachment. Instead, if we translate on* to use addEventListener the merge is fairly natural.
  • The patterns for web components to be passed actions is vague, but basically you can pass a function as a property or you can attach an event listener. Emitting events is suggested by some "pure" web component patterns, and of course an emitted event would not call the function Ember currently passes as a property. React has a similar issue. I suggest we install even listeners instead, making us compatible with the event pattern, but then also install a dispatcher as a property newElement.onmousewobble = newElement.onmousewobble || (details) => { newElement.dispatchEvent('mousewobble', new CustomElement('mousewobble', details)); } This would only support a single argument to be attached to the event as a property, but means the pattern of "DOM things emit with an event first" would apply to all web components, even those that want to leverage the more ergonomic API of calling a function over dispatching an event (at least by default).

For a webcomponent to dispatch an attached attribute action, it should use
`dispatchEvent`.

Glimmer components may (and this is specilation more than design) permit
Copy link
Member

Choose a reason for hiding this comment

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

s/specilation/speculation

@mixonic
Copy link
Sponsor Member Author

mixonic commented Jan 18, 2016

We discussed this at the F2F this weekend. I shared a few slides for discussion, you can read them here on slideshare, but they are nothing exciting.

Some resolution:

This RFC has helped identify difficult realities about how events listeners attach to different targets. Any <angle-thing> being considered a target. What events are emitted by native elements can be enumerated by looking the specs, and Ember components (glimmer components with angles) don't emit events (though they need other semantics) and so are exempt from listeners. Web components, however, have poorly defined behaviors. Some other frameworks have suggested you should pass a function to a web component on a prop that can be called, other advice (linked to in the RFC) suggest you should always use dispatchEvent. However in no case can Ember know what events may be emitted by a web component (or know if that component instead expects a function).

This is a fairly fundamental difficultly. The Ember team will continue to look at the web component space and try to derive recommendations and strategies for developers, other frameworks, and standards to improve this.

However in the near term:

  • The behavior of onclick= will continue to operate "as is". We may begin to document the behavior as an idiomatic pattern. I'll figure that out and make the relevant PRs.
  • There are legitimate cases when you want to add an event listener but there is no property-setter way to attach a listener. For example, onfocusin does not execute when the focusin event fires on an element. Web components may dispatch arbitrary custom events and not touch a property. To provide an escape value for a solution here, we will introduce an RFC for "element modifiers". This will allow the user-space creation of usage like: <div {{add-event-listener 'click' (action 'save')}}> to address the current lack.

Thanks for the time, thought, and commentary on this proposal! I hope to return with more ideas on how to improve built-in web component interoperability soon.

@mixonic mixonic closed this Jan 18, 2016
@mixonic
Copy link
Sponsor Member Author

mixonic commented Jan 25, 2016

@tomdale and @wycats have expressed some hesitancy about legitimizing onclick={{. We will revisit at the upcoming meeting.

@ghost
Copy link

ghost commented Jan 25, 2016

@cibernox mentioned to me that is already creeping into docs: http://emberjs.com/api/classes/Ember.Templates.helpers.html#method_action

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.

None yet