-
-
Notifications
You must be signed in to change notification settings - Fork 408
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
{{fn}}
Helper
#470
{{fn}}
Helper
#470
Conversation
A possible speedbump for this is the addon https://github.com/Serabe/ember-bind-helper. This addon provides an incompatible API to this RFC and is implemented by performing an AST transform The API for this RFC provides a clearer and cleaner mental model and a "just code" implementation, but we should understand that there is potential collisions for teams that are using this addon in their apps today. |
I'm not using any private API. I've already talked to some users of the addon about providing an upgrade path to this RFC. |
text/0000-bind-helper.md
Outdated
``` | ||
|
||
```js | ||
this.car.logout.bind(this.car, 1000, 'mile') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be this.car.logout.bind(this.honda, 1000, 'mile')
.
I'm very much in favor of this landing. I'll try to get a beta version of ember-bind-helper that facilitates current users stop using it. |
text/0000-bind-helper.md
Outdated
this.session.logout.bind(this.session) | ||
``` | ||
|
||
### Setting the `this` context and arguments |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
imo this is a little confusing since its the opposite of what the JavaScript bind function argument order is (wrt the context)
Can we make use of the array helper instead? Similar to how Angle Bracket does positional params (https://guides.emberjs.com/release/reference/syntax-conversion-guide/#toc_params-array)
{{bind this.car.drive context=this.honda params=(array 1000 'mile')}}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this would be making the use of this helper too verbose and a step backwards with respect to the action
helper.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we debated about this a bit, it comes down to “naming is hard”. What we want is a partial application helper. We’ve discussed:
partial
- used by aome other languages, but we can’t because it means something in hbsapply
- also used by other languages, but already used in JS, and means something very differentcurry
- could work, but then we have to explain the term. Also, may not be technically correct (currying produces a function that recieves exactly 1 argument, or something like that)papply
- easy to misinterpet if you don’t know what the term “partial application”partial-apply
- fairly verbose for a common task
bind
is the least bad choice IMO, it’s like if you made a bind
function with a slightly better API, but I also agree it’s not ideal and could be confusing. I think curry
is the second best out of those options, and am open to suggestions!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but isnt is confusing considered that in 90% of the cases {{bind
will not be used to bind context but @action
will bind context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got confused by that too, @luxferresum. If @action
will be the recommended way to bind context, I prefer a helper called curry
, apply-args
or something on that direction to partially apply arguments.
text/0000-bind-helper.md
Outdated
|
||
## Drawbacks | ||
|
||
Since this recommended design sets the `this` context to `null` using `function.prototype.bind` this could be confusing to junior developers if the function was not already bound (using something like an `@bound` decorator, JS binding, arrow functions, or the `bind` helper earlier in the stack). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/junior developers/new ember developers. IMO this context binding seems like a basic thing a framework should provide when template/JS class are already closely coupled. it's confusing if it's not out-of-box behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 to s/junior developers/new ember developers/
While it might look like a good idea to have automagic context binding, this behaviour is closes to how JS and other libs work. Reducing the difference between JS and HBS is a great goal, IMHO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When calling an action on a component don't you almost always want this
to be the component instance? would an @action
called with bind
be automatically have the this
context set to the component instance?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, @action
provides context binding. The magic that we’re discussing that is problematic is the fact that a helper or modifier can access this context implicitly, in order to bind it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In which case the "how we teach this" section is misleading:
For guides we would switch to recommending the bind helper to pass functions into components args and modifiers. In guides we would no longer recommend using the action helper based on the reasons listed in motivations.
This suggests you'd always want to use bind
over action
but I'd think that bind is only useful for the cases where you'd want a different context, which is (in my experience) never been a problem I've faced. Not to say that it's not a problem, but targetObject
was how this was done in the past and that wasn't very pleasant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rtablada, A little confused, what is the purpose of the default target=this
being applied at build time?
@viniciussbs Yeah, actions were a weird idiosyncrasy of the framework. bringing in plain JS bind, but keeping the idiosyncrasy makes things more weird for me, and just seems like an unnecessary hat tip to backwards compatibility. Wouldn’t it make more sense to just default to context=this
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@mehulkar defaulting to context=this
is more harmful than helpful. You might not be passing a function defined in the template context, but one coming from a service or a model, like {{bind this.model.save}}
. That is the reason in ember-bind-helper
we default to the everything but the last part of the call invocation (in the example this.model
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When using bind
with a context and an @action
decorator which one wins?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@action
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the reason in ember-bind-helper we default to the everything but the last part of the call invocation
@Serabe yeah this would be fine (and better) also 👍 . Defaulting to null
is what I'm objecting to.
text/0000-bind-helper.md
Outdated
### Setting the `this` context | ||
|
||
```hbs | ||
{{{bind this.session.logout context=this.anotherSession}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hope the tiple braces are by accident?
text/0000-bind-helper.md
Outdated
### Setting the `this` context | ||
|
||
```hbs | ||
{{{bind this.session.logout context=this.anotherSession}}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think context
is a pretty overloaded word that doesn't really convey a lot of meaning. Why don't we use thisArg
to make it less ambiguous and match the EMCA spec's naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1, or just this
?
I use @bound
method() {
// method on a service
} <button onclick={{bind myService.method}}>
Call a method on a service
</button> |
@viniciussbs in that case you don't even need the |
@viniciussbs for some clarification we would recommend @action
method() {
// method on a service
} <button {{on "click" this.myService.method}}>
Call a method on a service
</button> |
Quick update: if you are using |
@rtablada So, if using |
Except when you supply your own context - this means you need to look at both the template and the code to understand what the context of the ultimate action is. Is there a solution where the context is always defined in one place or the other? |
We discussed this RFC at the face-to-face last week, and with @rtablada's permission I've updated it to reflect the feedback from that meeting and the current state of things. Our main feeling is that the mismatch between the semantics of We discussed many, many names for this new helper. I outlined some of the alternatives we went over in the RFC, along with the reasoning and pros/cons for them. There is a decent amount of support for
Overall there was more support for it than |
@pzuraq the examples still have the
This is such a common operation that maybe there's a way to express it with a more "native" feeling syntax. Under the covers this would be compiled to use |
@jonnii thanks for catching those! That was unintentional, I'll update soon. So, the thing is, you examples will be possible soon (if they aren't already). They just have a bit of a different syntax: <button onclick={{this.increment}}>Click</button>
<button {{on "click" (this.add 1 2)}}>Click</button> Glimmer templates have their own way of invoking functions (helpers), which is different from JavaScript. I don't think it makes much sense for us to try to embed JS in our templates, that would definitely be going the JSX route. The issue is though, the above runs those helpers immediately, because it's an inline function call. What we want is to have a syntax where we return a new function that is called later. In React/JSX they have this same problem, and they encourage arrow functions in templates: <button onClick={(e) => this.deleteRow(id, e)}>Delete Row</button> We briefly discussed adding something like arrow functions to Glimmer templates, something like: <button {{on "click" (|e| => (this.add 1 2))}}>Click</button> But you can see this is actually still pretty verbose, and it opens up a whole can of worms. I think it's a feature we can definitely consider in the future, but in the meantime it makes more sense to have a helper which passes back a bound function. |
Co-Authored-By: rtablada <[email protected]>
Would the name |
After more discussion, a few more options have come up for us to consider:
<div {{on "click" (fn this.save model)}}></div>
<div {{on "click" (> this.save model)}}></div>
|
I was at peace with
|
Alignment with Rust can only be a good thing. We've got 😆 |
The RFC has been updated to use |
I still prefer |
Fwiw, I also prefer |
I still prefer |
@bendemboski heh, well, strictly speaking, it partially applies a function rather than currying it. And (at least in my view) we probably shouldn't encourage the confusion of the two terms, as they are both independently useful concepts! Edit: clarification for those who might not be familiar with the terms— Partial application takes a function which takes m arguments and applies the n arguments you might supply to it and returns a function that expects m - n arguments. So, using lodash's function addFourNumbers(a, b, c, d) {
return a + b + c + d;
}
let addTwoNumbers = _.partial(addFourNumbers, [1, 2]); Here function addTwoNumbers(c, d) {
return addFourNumbers(1, 2, c, d);
} Currying takes a function which takes m arguments and turns it into a series of m functions which take one argument each. So, again using lodash, this time with function addFourNumbers(a, b, c, d) {
return a + b + c +d;
}
let curriedAddFourNumbers = _.curry(addFourNumbers, 4); This is equivalent to this: let curriedAddFourNumbers = a => b => c => d => a + b + c + d; You can see how these two things are useful together (and a number of languages automatically curry their functions as a powerful complement to partial application, so people often associate them), but the differences between them are useful to understand. |
@chriskrycho yeah, that's a fair point. I was sorta relying on the fact that I believe most people kinda blur or confuse the two notions, so calling it currying would immediately invoke the idea of partial application, and since there's no sensible way (that I can think of) that |
@bendemboski yeah, I actually can think of ways that you might use it, but they're used together often enough to cause exactly that confusion. (I edited in an example for folks that are following along via email, which should hopefully show why they're often used together and why they're worth keeping conceptually distinct!) I definitely think keeping it correct is a worthwhile goal: it's almost never a good idea to intentionally name something in a way that runs contrary to established conventions or definitions. |
text/0000-bind-helper.md
Outdated
return function() { | ||
this.add.call(this, 1, 2); | ||
} | ||
``` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got a bit confused by looking at these examples, as they make it look like you have to pass all expected arguments, rather than partially applying them. Wouldn't the correct JS equivalent of {{fn this.add 1 2}}
look more like this?
return function(...args) {
this.add.call(this, 1, 2, ...args);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah actually that’s totally correct, we will update them!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pzuraq it looks like this change didn't make it into the final version of the RFC text?
Another vote for |
Great discourse! Maybe we can ask a non-ember person what they think
|
@sheriffderek I think these examples are for folks who do use |
updated link to RFC470 |
rendered