Support target option for closure actions

[Panman82]

It seems when using a closure(action) the target option is not respected in the same way as with the "traditional" helper {{action}}. My main need is for the called action/function to have the this context of object itself, not the caller. The most common use case is when you're trying to pass in an action into an ember component. Ex:

{{#my-button click=(action someObj.funcName target=someObj)}}
  Do Action
{{/my-button}}

However, at the same time if you're able to use the "traditional" helper, then it works. Ex:

<button {{action someObj.funcName target=someObj}}>
  Do Action
</button>

I have a working example in an Ember Twiddle. But for reference here, the called function would look something like:

let someObj = Ember.Object.create({
  propName: 'foobar',
  funcName() {
    // Do something with `this`
    this.get('propName');
  }
});

In chatting with @rwjblue and company in the Slack community, apparently this can be handled in the action AST file, which should then be able to eliminate some "ugly code" in the action modifier. I'm still trying to wrap my head around the internals and AST stuff, but at least there is direction in where to start.

[Panman82]

Looking back at more Slack chat, Robert provided even more direction which is great!

https://embercommunity.slack.com/archives/-help/p1476381597028760

https://github.com/emberjs/ember.js/blob/v2.9.0-beta.5/packages/ember-template-compiler/lib/plugins/transform-action-syntax.js#L65-L67 needs to be rewritten to search the hash arguments for target and use that if present

then we can completely remove https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/modifiers/action.js#L95-L106

/CC @sukima since you were involved with that particular convo.

[zackthehuman]

This makes sense to me. If I understand the suggestion -- basically we would change the AST plugin to check for a target hash arg, and if there isn't one we just create one that does target=this.

[zackthehuman]

/cc @sukima

[mixonic]

The target= option specifies what object to look to for an action, not what context to bind the resulting function too. The actions has specifies the template facing API of an object, for example if you want to target a service actions can be added:

/* usage:
 *
 * {{my-button click=(action 'funcName' target=someService)}}
 */
export default Ember.Service.extend({
  actions: {
    funcName() {
      /* something */
    }
  }
});

I'm a little wary of making target= into a general purpose bind helper. @Panman8201 @zackthehuman is there a use case that couldn't be handled with actions hash currently?

I agree we can improve the ergonomics of the actions hash, and I think everyone is eager to do that once we can adopt decorators. If the use-case here is that devs just don't like the ergonomics though, I'm worried about forking the convention of where template-facing actions are.

[zackthehuman]

@mixonic I thought the issue here was that there is an inconsistency between closure actions and modifier actions in how target is used differs between them.

In any case, if target didn't work like bind I believe it would have bad consequences since you can pass path references as the "action function" argument. Without specifying a target, the function will be called with the wrong context which seems bad/confusing.

[Panman82]

I agree with @zackthehuman in that I thought the issue was to fix an inconsistency. The actions hash + string usage is something I didn't know worked at first, until troubleshooting further for this issue/case. While I could refactor to make that usage work, I think this should still be fixed.

[mixonic]

Without specifying a target, the function will be called with the wrong context which seems bad/confusing.

Written as (action someFunc) someFunc will execute just like if you called someFunc(). This isn't wrong or right, it is just what JavaScript does. In general it isn't expected that devs are passing raw functions into the action helper, it is expected that they are passing either strings or already created actions yielded from elsewhere.

That target bound a function with the classic action helper seems like it was perhaps an implementation quirk.

@zackthehuman @Panman8201 you both sounds like you have use-cases for the binding behavior though- can you provide an example of how you use it in an application today, or would like to be able to use it?

[sukima]

@mixonic Say I have an Ember data model which has helper functions on it:

import Ember from 'ember';
import DS from `ember-data`;

const { Model, attr } = DS;
const { get } = Ember;

export default Model.extend({
  foo: attr({defaultValue() { return []; }}),

  addNewFoo() {
    get(this, 'foo').pushObject({bar: 'bar'});
  },

  moveFooUp(index) {
    let foo = get(this, 'foo');
    let orig = foo.objectAt(index);
    foo.removeAt(index);
    foo.insertAt(index - 2, orig);
  },

  moveFooDown(index) {
    let foo = get(this, 'foo');
    let orig = foo.objectAt(index);
    foo.removeAt(index);
    foo.insertAt(index, orig);
  }
});

So in a view I might have:

{{#each model as |myModel|}}
  {{#each myModel.foo as |theFoo index|}}
    <div>{{theFoo.bar}}</div>
    <div class="user-actions">
      <button onclick={{action (action model.moveFooUp target=model) index}}>^</button>
      <button onclick={{action (action model.moveFooDown target=model) index}}>V</button>
      <button onclick={{action (action theFoo.removeAt target=theFoo) index}}>X</button>
    </div>
  {{/each}}
  <button onclick={{action (action model.addNewFoo target=model)}}>Add another foo</button>
{{/each}}

[zackthehuman]

@mixonic (action someFunc) will call someFunc.call(this, ...) which means it is implicitly bound to this. It's not the same as calling a free function someFunc().

Here is a twiddle to illustrate what I mean: https://ember-twiddle.com/#/a61ed2dedc6e6e0729b4a2d7b60e5ad9

Edit: I don't have a horse in this race, if this is the intended behavior I'm happy to close my PR.

[rwjblue]

@mixonic - This is not related to adding a new functionality (as you seem to be implying), it is related to making (action and {{action support the same things.

One small example: <button {{action bar.foo target=bar}}></button> will call bar.foo() , but {{my-button click=(action bar.foo target=bar)}} will call foo.call(<template this context>).

Additionally, take a look at the following docs which suggest that action (the general concept) should support this:

• https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/helpers/action.js#L22-L40
• https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/helpers/action.js#L81-L83
• https://github.com/emberjs/ember.js/blob/master/packages/ember-glimmer/lib/helpers/action.js#L240-L258

Perhaps, your position is that we should never have supported <button {{action bar.foo target=bar}}>, but that ship has sailed on that at this point and we are in a super bizarre place where we document both forms of action together but they do not actually support the same things.

---

All of that being said, this PR also has merits on its own. Removing the "maybe implicit or explicit target" behavior in action code (both modifier and helper) is a large win too.

[mixonic]

Right. I totally understand there is an inconsistency between element-space action and closure actions as regards target. It isn't the only inconsistency (for example closure actions do not bubble) and it might seem unintentional. I agree the silent failing case we have today, where a developer might migrate from an element-space action to a closure action and with copy-pasta of target expecting consistent behavior but not get it, is less than ideal and can be improved.

However I strongly believe the use-case outlined above (addressing methods on a model) is an anti-pattern in Ember app development. In this light, the behavior of classic actions can be considered something of a bug, and I would not like to copy that behavior forward into closure actions. In the medium-term element-space actions will be deprecated in Ember and in the long term they will be removed, however the closure action API will remain with Ember for what I expect to be a long time. Let's only bring with us what we want.

There is no documentation suggesting target is a context

Of the three documentation snippets @rwjblue linked to, only a single one implies that the target would be used as the context for function evaluation. The example code for it, however, uses a string action name and not a function. There is no documentation I'm aware of that describes target as a context argument for functions passed in.

In fact the documentation is explicit about what target does in the intro:

• target=someProperty will look to someProperty instead of the current
  context for the actions hash. This can be useful when targeting a
  service for actions.

It will look to it for the actions hash. Not call a function with the context.

Why avoiding actions is an anti-pattern

Many experienced devs grow to be annoyed at the actions design in Ember. I totally agree the ergonomics are less than ideal, however they can be improved with time and the design serves a purpose.

The common footgun actions is intended to help with is a conflict in template-facing actions and lifecycle hooks. For example:

export default CustomModel.extend({
  destroy() {
    this.get('ajax').del(`/models/${this.get('id')}`);
    this.set('isDeleted', true);
  }
}):

When CustomModel is being defined, there isn't a way to know if a user has unwittingly used a method name that conflicts with a hook, or if they intended to subclass that behavior. What was the goal in the above code? Override the destroy lifecycle hook, or add an action?

If target is permitted to create a context the above method can be called with:

{{my-button click=(action model.destroy target=model)}}

Not only has the user fallen intro a trap we can prevent them falling into (stomping destroy), but they've also needed to type model out twice. Given that this could be a service or something with a complex name it only invites typos and reduces readability to encourage this.

Compare this to the actions system, which works today:

export default CustomModel.extend({
  actions: {
    destroy() {
      this.get('ajax').del(`/models/${this.get('id')}`);
      this.set('isDeleted', true);
    }
  }
}):

{{my-button click=(action 'destroy' target=model)}}

This had no repetition and does not conflict with the lifecycle hook (destroy) on the model.

For actions to work well the framework needs to encourage it at all times. Even though there may be cases where passing a context may work safely, we need something robust against known failure cases. We want all apps to use actions to expose API to templates.

But the ergonomics suck

I agree that actions seems dated. I expect what we would like to do once ES classes are supported is use decorators. For example:

export default CustomModel extends {
  @action
  destroy() {
    /* some logic */
  }
}

A decorator would allow us to assert the user hasn't fallen prey to a common footgun. For example we can check that there is no method named destroy on the super-class, or that if there is a method on the super-class that it is also an @action (actions can only override actions). In the above case because destroy is indeed defined up the chain, we can assert that an action is stomping a lifecycle hook and notify the user appropriately.

Once this system is in place we can also have actions create closures with their context. Because the context is packaged into the action, target would not be needed to specify a context. This removes the duplication then action addresses a method on the model. I think (action model.someAction) would be viable at that point. Perhaps the action helper itself isn't needed.

I think we can improve this and will improve this, but I'm not eager to encourage developers to do something other than use the current actions as is designed today.

What I think we should do

I don't suggest that nothing should be done. There is definitely a subtle inconsistency between the
two APIs and we can improve the failure mode.

• In closure actions where we deal with raw functions being passed in (I think here?) we should assert if target is also passed, suggesting that the user instead use string actions.
• In closure actions if there is an implicit context of this (which from scanning the code I can't point to exactly but I believe it since others have raised it) we should change that to be null. Basically when a function was passed to the action target passed to run should always be null. If the method has a closure it will still evaluate as expected, only the implicit cases will see a change in behavior.

[zackthehuman]

I agree that we should fix the implicit binding and use null instead, which aligns action semantics more with normal JS function semantics.

[Serabe]

Do you agree on @zackthehuman last comment?

[mixonic]

Yup, we are on the same page. Much of @zackthehuman's PR was cleanup that I think still should be applied.

[pixelhandler]

@Panman8201 @zackthehuman @mixonic @rwjblue is this still an issue, perhaps we should close or create a new reproduction of this, what do you think?

[pixelhandler]

Per our triage policy I'll close this out for now, feel free to reopen if the issue persists on the current release.

• https://github.com/emberjs/ember.js/blob/master/CONTRIBUTING.md#triaging-policy