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

Add improved actions RFC #50

Merged
merged 1 commit into from May 11, 2015

Conversation

Projects
None yet
7 participants
@mixonic
Copy link
Member

commented May 7, 2015

would be written using a closure action as:

```hbs
<button {{action (action "submit") on="click"}}>Save</button>

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

This is pretty confusing, why double wrapping in action? What does the closure wrapped action get attached to?

This comment has been minimized.

Copy link
@mixonic

mixonic May 7, 2015

Author Member

Hm. It is not double wrapped- the return value of (action "submit") is a function, which the element-space {{action helper calls.

String-based actions bubble like before. Function actions are closures to the scope they were created in.

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member
  • <button {{action 'submit'}}>Save</button> would invoke the local submit action as the button elements on click handler.
  • <button {{action (action "submit")}}>Save</button> would wrap the local submit action in a function, then attach that function to the button elements on-click handler.

So, yes, technically it would work, but why would you ever do this? If you are calling an action from your own local scope (which would have to be true for (action "submit") to work), why would you then prefix with {{action again?

This comment has been minimized.

Copy link
@mixonic

mixonic May 7, 2015

Author Member

We discussed, will update in RFC to clarify the steps happening. Robert is correct that these cases would have identical behavior, that was what I intended to show.

```js
export default Ember.Component.extend({
click: function(){
this.attrs.action === "submit";

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

This seems like a comment informing the reader of what happens when this.sendAction() is called below, can it be made into a JS comment so the highlighting makes it clearer?

```js
export default Ember.Component.extend({
click: function(){
typeof this.attrs.action === "function";

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

Can make this a comment or add a comment explaining why it is here?

```

```hbs
{{! app/components/my-button/template.hbs }}

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

Seems like this should actually be in {{! app/templates/index.js }} since it is invoking my-button (and passing the closure wrapped submit action in to the {{my-button}} component).


### Hole punching with a closure-based action

The current system of action bubbleing falls down quickly when you want to pass a message through multiple

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

I am not certain, but I think this should be bubbling.

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

Also, you might mention it as "manual action bubbling". @danmcclain wrote a nice article about the current technique at http://reefpoints.dockyard.com/2015/01/28/bubbling-actions-through-components.html...

@mixonic mixonic force-pushed the action branch from 4a8c44e to 9f42fe1 May 7, 2015


```hbs
{{! app/components/my-form/template.hbs }}
{{yield save (action 'reset')}}

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

Should this be attrs.save?

The closure usage is a new, perhaps `action` is not the right word. However the two
behaviors are pretty similar in their conceptual behavior.

* `{{action` in an element space attaches an action from the current scope to an event on the element,

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

"attaches an action name" (not the action itself, which I think of as the action function)

* `(action` closes over an action from the current scope so it can be attached as an action later.

Additionally, there may be developers who still have `{{action someActionName}}` instead
of the quoted version. This is long deprecated, but these apps may see some

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

AFAIK, it is actually unsupported since 1.10 or 1.11. We do support bound action names (where the name resolves to a string when clicked) though, and that is not deprecated (and I think it should stay).

@mixonic mixonic force-pushed the action branch from 9f42fe1 to 124961e May 7, 2015

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 7, 2015

I am hugely 👍 on this RFC, when can I have it?

@workmanw

This comment has been minimized.

Copy link
Contributor

commented May 7, 2015

What about target and action arguments? Will those be supported? Example:

{{user-inline-edit submit=(action "saveUser" userModel target="bulkSaveManger")}}
@mixonic

This comment has been minimized.

Copy link
Member Author

commented May 7, 2015

@workmanw interesting. It is technically possible. target would be specifying an alternative target for the closure. I will add.

This does raise the issue of <button {{action someClosureAction target=otherComponent}}> which should just throw IMO. You cannot have a closure action and a target for that action defined at attach-time.

@mixonic mixonic force-pushed the action branch from 124961e to d7f2128 May 7, 2015

@workmanw

This comment has been minimized.

Copy link
Contributor

commented May 7, 2015

@mixonic I'm not sure how others feel, but for our app this would be greatly beneficial. One of our existing pain points with the current action system is the need for a single component to have different targets for different actions. Wrapping up the target in the action sub-expression would save us a lot of "action daisy chaining".

Thanks for this RFC! Super excited for this.

@mixonic mixonic force-pushed the action branch from d7f2128 to a7e59bc May 7, 2015

@workmanw

This comment has been minimized.

Copy link
Contributor

commented May 7, 2015

👍 Thank you.

@mixonic mixonic force-pushed the action branch 2 times, most recently from c525e0b to 0e744e4 May 7, 2015


```hbs
{{! app/components/my-form/template.hbs }}
{{my-button on-click=attrs.submit}}

This comment has been minimized.

Copy link
@drogus

drogus May 7, 2015

Would {{my-button on-click=(action 'submit')}} work here as well?

This comment has been minimized.

Copy link
@mixonic

mixonic May 7, 2015

Author Member

{{my-button on-click=(action 'submit')}} would target the submit action on MyForm. The current scope is always what is looked to for an (action string.

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 7, 2015

Member

Only if you wanted to provide my-button with the "submit" action from my-form. As it is written here, my-button is getting the wrapped action directly from index controller/route, and you do not need to handle the action and this.sendAction again (the current manually bubbling).

This comment has been minimized.

Copy link
@drogus

drogus May 7, 2015

Ah, right, I have misunderstood the use case here, thanks for explanation!

@mixonic mixonic force-pushed the action branch from 150a47f to f3cdde7 May 8, 2015

closed over functions that can be passed between components and passed
the action handlers.

See [this example JSBin from @rwjblue](http://emberjs.jsbin.com/rwjblue/223/edit?html,js,output)

This comment has been minimized.

Copy link
@rwjblue

rwjblue May 8, 2015

Member

I updated the JSBin to work (using 1.11.3): http://emberjs.jsbin.com/rwjblue/466/edit?html,js,output

The original doesn't work (doesn't use template compiler).

@mixonic mixonic force-pushed the action branch from f3cdde7 to bdf60aa May 10, 2015

@mixonic mixonic referenced this pull request May 10, 2015

Merged

[FEATURE] Improved closure based actions per RFC #50 #11089

2 of 2 tasks complete

@mixonic mixonic force-pushed the action branch from bdf60aa to 00ac268 May 10, 2015

@mixonic

This comment has been minimized.

Copy link
Member Author

commented May 10, 2015

Updated with a description of value=.

Seems good to merge.

@rwjblue

This comment has been minimized.

Copy link
Member

commented May 10, 2015

:👍:

@lxcodes

This comment has been minimized.

Copy link

commented May 11, 2015

👍 What would yielding multiple actions look like?

Current example:

{{! app/components/my-form/template.hbs }}
{{yield attrs.save (action 'reset')}}

Multiple yield?:

{{! app/components/my-form/template.hbs }}
{{yield attrs.save (action 'reset') (action 'update')}}

or

{{! app/components/my-form/template.hbs }}
{{yield attrs.save (action 'reset' 'update')}}
@mixonic

This comment has been minimized.

Copy link
Member Author

commented May 11, 2015

@al3x-edge like:

{{yield attrs.save (action 'reset') (action 'update')}}

Your third example would curry the argument update onto the action. When this.attrs.reset() was called (with no argument), the handler would receive "update" as the first argument.

@lxcodes

This comment has been minimized.

Copy link

commented May 11, 2015

@mixonic Ah. Makes sense. Didn't read well enough apparently -- Thanks Matt

@mgenev

This comment has been minimized.

Copy link

commented May 11, 2015

Does this address all the problems expressed in this RFC? #42

If yes, yay, anything that fixes the most annoying part of ember - having to repeat, redeclare and resend actions every tier is very welcome.

If not, meh, because there's a risk here of actions becoming even more confusing. This syntax is bad: {{action (action "submit") on="click"}}

{{action (action ??

Overall if actions were based on javascript events and bubbled naturally like them, a lot of these complications could be avoided.

@mixonic

This comment has been minimized.

Copy link
Member Author

commented May 11, 2015

@mgenev it mitigates the challenge you pointed out in the Summary, although how it fixes that problem is very different than the solutions suggested in that discussion.

What used to be:

{{! app/index/template.hbs }}
{{my-component openModal="openModal"}}
{{! app/components/my-component/template.hbs }}
<button {{action 'popSomething'}}>
/// app/components/my-component/component.js
import Ember from "ember";

export default Ember.Component.extend({
  actions: {
    popSomething() {
      this.sendAction('openModal');
    }
  }
});

Is now simply:

{{! app/index/template.hbs }}
{{my-component openModal=(action "openModal")}}
{{! app/components/my-component/template.hbs }}
<button {{action openModal}}>

However I'm not sure this addresses your desire to have "openModal" be an action on the route reachable from any arbitrary point in the component hierarchy. Closure actions read the function off this.actions and call it- they do not use send at all and thus will never bubble.

For the specific case you laid out, I would strongly suggest using a service:

{{! app/components/my-component/template.hbs }}
<button {{action (action 'open' target=modalService)}}>
/// app/components/my-component/component.js
import Ember from "ember";

export default Ember.Component.extend({
  modalService: Ember.inject.service('modal')
});
/// app/services/modal.js
import Ember from "ember";

export default Ember.Service.extend(Ember.Evented, {
  actions: {
    open() {
      this.trigger('didOpen');
    }
  }
});
/// app/components/my-modal/component.js
import Ember from "ember";

export default Ember.Component.extend({
  modalService: Ember.inject.service('modal'),
  init() {
    this._super.apply(this, arguments);
    this._onOpen = () => {
      // do something to display popup
    };
    this.get('modalService').on('didOpen', this_onOpen);
  },
  destroy() {
    this.get('modalService').off('didOpen', this._onOpen);
    this._super.apply(this, arguments);
  }
});

In this example, the my-modal component just reflects the UI state of the modalService. The action open can be called from any component that has the service available.

It is off topic for this RFC, but I believe there are at least two follow-up RFCs that would help make this pattern easier:

  • Managing the event subscription to didOpen manually is error prone. @igorT and I were just talking about making Ember.on('modalService.didOpen', () => {}) work, which would remove the need for manual setup and teardown.
  • <button {{action (action 'open' target=modalService)}}> screams for a better way to attach a closure action to a click event. We've toyed with <button on-click={{action 'open' target-modalService}}> but have not yet reconciled that with components.
  • Oh yeah, maybe all services should be evented. I'm starting to use that mixin a ton.
  • There is un-RFC'd discussion of making outlet.foo a way to access the routable component of a given component. This provides the "skip to the top" functionality you want without a concept of global bubbling. You would just make the target of the action outlet.

Still improvements to go, but I'm feeling much better about the patterns we can create with these new actions and with the direction our conversations are going.

@mgenev

This comment has been minimized.

Copy link

commented May 11, 2015

@mixonic thank you for the great detailed explanation and examples. Being able to omit the redundant sending of the intermediate action is indeed a big improvement.
Regarding services, in the case of a modal, I can see how it makes a lot of sense to have a modal service.

What about other instances though in which I'm simply nesting components 4-5-6 levels deep in a route, which I do find myself doing now in the "everything's a component" architecture. Am I to create a service for every route then? This strikes me as a hassle. I'd rather have some kind of global proxy service or a pub-sub pattern which allows me to take actions to the top in every case...

Perhaps the future RFC's you mention would make this pattern easier indeed, I'd be happy to see future thoughts there.

@mixonic

This comment has been minimized.

Copy link
Member Author

commented May 11, 2015

@mgenev I find that most cases where I need access to the "top" level scope (by that we usually mean the controller today, in the future the routable component) can be handled by using yield:

{{! app/index/template.hbs }}
{{my-component}}
{{! app/components/my-component/template.hbs }}
{{my-other-component passedVal=fromComponent}}
{{! app/components/my-component/template.hbs }}
<button {{action omgHowDoIGetToController}}></button>

Composing these component with yield keeps access to the parent scope easier:

{{! app/index/template.hbs }}
{{#my-component as |val|}}
  {{#my-other-component passedVal=val}}
    <button {{action omgHowDoIGetToController}}></button>
  {{/my-other-component}}
{{/my-component}}
{{! app/components/my-component/template.hbs }}
{{yield fromComponent}}
{{! app/components/my-component/template.hbs }}
{{yield}}

But I definitely acknowledge this isn't always viable. I think the outlet. API is the best bet for accessing "top level" things when components are deeply nested. It is always going be a bit awkward, because the ask is a tough one. A deeply nested component being aware of the ambient state for the whole application violates the goal of having components be scoped, isolated things that are portable.

rwjblue added a commit that referenced this pull request May 11, 2015

Merge pull request #50 from emberjs/action
Add improved actions RFC

@rwjblue rwjblue merged commit 6815eb2 into master May 11, 2015

@rwjblue rwjblue deleted the action branch May 11, 2015

@mgenev

This comment has been minimized.

Copy link

commented May 11, 2015

@mixonic thanks, I'll explore the pattern with the block component and yielding, @runspired was also telling me that as a solution in the slack.

For reference, here's how Aurelia solves these issues very simply:
http://aurelia.io/docs.html#event-modes
http://aurelia.io/docs.html#eventing

@rabhat

This comment has been minimized.

Copy link

commented Jun 15, 2015

@mixonic

I have tried to implement modal dialogue using service pattern as you mentioned in your post, but i am not able to achieve the same ,it would be great help if you please share the working code in JSBIN

Thanks

twokul pushed a commit to twokul/rfcs that referenced this pull request Jan 11, 2018

Merge pull request emberjs#50 from GavinJoyce/gj/production-code-stri…
…pping

Ember CLI: Production code stripping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.