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

set helper #594

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

set helper #594

wants to merge 4 commits into from

Conversation

luxzeitlos
Copy link

@btecu
Copy link

btecu commented Feb 20, 2020

Why would @model.someProperty not be valid use case?

@pzuraq
Copy link
Contributor

pzuraq commented Feb 20, 2020

@btecu @model.someProperty would be valid, but @model alone would not. The issue is we don't know what property we're setting the value on. This is something mut can do today that is pretty weird, and causes a lot of pain with in the VM.

Copy link

@nightire nightire left a comment

Choose a reason for hiding this comment

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

One thing always confuses me and my colleagues is that how ember.js determined the value passed to the one-argument version of {{set}} helper?

When people doing this:

<input {{on "change" (set this.foo)}}>

does this.foo will be set as event.target.value? Or people should do this:

<Input @value={{@value}} {{on "change" (set this.foo @value)}}>

to explicitly set the value bound to the <input> element?

If the latter one is correct, then the two-arguments version is definitely necessary.

text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
text/0000-set-helper.md Outdated Show resolved Hide resolved
Comment on lines +63 to +69
The first argument must *always* have a context. So `{{set foo}}` or `{{set @foo}}` is not valid.
This aligns with [RFC 0308](https://github.com/emberjs/rfcs/blob/master/text/0308-deprecate-property-lookup-fallback.md).

The fact that the `set` helper somehow needs to know the *context* and the *name* of the
argument it receives makes it magic syntax that can not be used by a userland helper without
a AST transformation. But the need for the helper seems big enough to justify this, especially
when the same magic syntax is already used by the `mut` helper.
Copy link
Contributor

@chriskrycho chriskrycho Feb 21, 2020

Choose a reason for hiding this comment

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

What about this scenario? Edit: this is all misguided, see further comments below.

{{#each @values as |value|}}
  <FancyInput @onChange={{set value}} />
{{/each}}

This seems like a very common use case, and one that the current proposed design would rule out.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps this is part of the reason this doesn't fully replace {{mut}}! If so, that's probably worth making explicit in the text of the RFC.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just realized that this exact behavior probably doesn't work at all regardless of this. A better example would be something like:

{{#each @users as |user|}}
  <FancyInput @onChange={{set user.name}} />
{{/each}}

The alternative, less magical version as suggested in another comment would just be:

{{#each @users as |user|}}
  <FancyInput @onChange={{set user 'name'}} />
{{/each}}

I could see an argument either way here in terms of the brevity-vs.-magic tradeoff. Apologies for the confusion!

Copy link
Author

Choose a reason for hiding this comment

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

exactly. {{set user.name}} would work. Also this does not work currently (just tested it):

{{#each @values as |value|}}
  <FancyInput @onChange={{fn (mut value)}} />
{{/each}}

I generally would want {{set user.name}} and not {{set user "name"}}. But indeed, both would work.

```

The new `set` helper can be used instead of the old `{{fn (mut ..)}}` combination.
However this RFC does not deprecate `{{mut}}`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not deprecate {{mut}}, preferably with a timeline where there are one or more minor releases between the time when {{set}} is added and {{mut}} is deprecated so that addons can migrate without causing unactionable noise for consumers.

If there's a reason we cannot or should not deprecate {{mut}}, I think (a) we should spell that out here, but also (b) it undercuts the motivation for this RFC. We end up in a spot where there are two ways of doing something, one of them likely preferred/recommended but the other still supported and without a clear indication that you should move away from it to the new preferred/recommended approach. If there are gaps in functionality from {{mut}} which we believe need to be covered, it's fine to deliver them separately, but that should be expressly called out in the text of this RFC.

Copy link
Author

Choose a reason for hiding this comment

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

Honestly I think there are maybe gaps, be we need to find them. We have no replacement for the mutable attribute syntax. I think long term we should deprecate mut, but this will be a first step in that direction. A future RFC should deprecate mut, and maybe additional RFCs are required for the gaps. I will try to make this clear in the RFC. Thanks for pointing it out.

Comment on lines 26 to 40
When used to pass an argument as in `<MyComponent @foo={{mut this.bar}}>` or
`{{my-component foo=(mut this.bar)}}` it will do nothing for a glimmer component
but provide a box with a `value` and an `update` function on the private `attrs`
property of a classic component.

However the more common use-case is to pass the result of `mut` to either the
`{{action}}` or the `{{fn}}` helper:

```hbs
<SelectCountry @update={{fn (mut this.country)}} />
<button {{on "click" (fn (mut this.name) "")}}>delete name</button>
```

However many people would expect `{{fn (mut this.foo)}}` to be the same as
`{{mut this.foo}}` in all cases, which can lead to confusion.
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you expand this with more detail on what exactly the difference is? This is a distinction I've loaded up into my head a number of times, but since I make a point not to reach for {{mut}} I inevitably forget it—and it's not clear to me what the actual difference is in practice and effect between these two. If you could show in these examples how they differ in behavior, that would help not only me but everyone else in evaluating this RFC!

Copy link
Author

Choose a reason for hiding this comment

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

I tried to explain the problem a bit. I would love some feedback if its understandable.

Comment on lines +68 to +69
a AST transformation. But the need for the helper seems big enough to justify this, especially
when the same magic syntax is already used by the `mut` helper.
Copy link
Contributor

@chriskrycho chriskrycho Feb 21, 2020

Choose a reason for hiding this comment

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

I actually disagree with the idea that because mut does this magic, so should set. Indeed, part of the reason that mut is confusing is that it does do magical things and doesn't behave like other helpers. Accordingly, I suggest—

  • that you add an alternative where we only add the two-argument version add a version which is not magical and explicitly takes both a context and a property key name
  • that you mark this design question as an Unanswered Question

Copy link
Contributor

Choose a reason for hiding this comment

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

Strongly agree. Especially I think that set helper should have a similar syntax as the existing get helper, which is {{get context "prop"}}.

Comment on lines +78 to +86
The existing `mut` helper allows to receive the result of the `get` helper to create a
[dynamic attribute binding](https://guides.emberjs.com/release/components/built-in-components/#toc_binding-dynamic-attribute):

```hbs
<Input @value={{mut (get this.person this.field)}}
```

This use-case is *not* covered by the new `set` helper, however the existing `mut` helper
can still be used for this.
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be an argument for the context-and-key alternative, {{set context fieldName}}, as that would handily solve this and generalize to the other cases as well.

@luxzeitlos
Copy link
Author

@nightire

One thing always confuses me and my colleagues is that how ember.js determined the value passed to the one-argument version of {{set}} helper?

It does not. So when you do this:

<input {{on "change" (set this.foo)}}>

It will set this.foo to the Event.

This one would actually work. However I would strongly recommend you to not do this!

<Input @value={{@value}} {{on "change" (set this.foo @value)}}>

It only works because change fires after input. So this will be really problematic:

<Input @value={{this.value}} {{on "input" (set this.foo @value)}}>

Then this.foo will always be one version behind this.value.

But this is a limitation of the <Input> component, not the set (or mut) helper.
I agree that this should be addressed. But this is out of scope here. A nice showcase
how this could be adressed is ember-box. Or we could add an @update parameter to
<Input> that would allow <Input @value={{this.value}} @update={{set this.value}} />.

But this is really out of scope here. Actually one of the reasons mut is problematic
is that it does a lot of magic and tries to fix to many problems.

@mehulkar
Copy link
Contributor

mehulkar commented Feb 21, 2020

I'm very much in favor of set over mut, but a couple concerns:

  1. mut and set both seem like a "shortcut" to just defining an action in JS and passing that around in the template. Is that correct or are there other reasons for using these?
  2. If (1) is correct, then the main usage of these helpers would be for Template Only components.

Given (1) and (2) , requiring a context in order to use this new helper begs the question: why not remove the helpers entirely and just require the action? Is it really so necessary to provide a primitive that generates functions in the template layer?

For what it's worth, I've never used mut in all the time I've been using Ember, mostly because I never understood how to use it, and I don't feel like I've not been able to do something.

@luxzeitlos
Copy link
Author

@mehulkar I would say yes to 1) but no to 2).

A good real-world example is using set with power-select:

<PowerSelect @selected={{this.destination}} @options={{this.cities}} @onChange={{set this.destination}} as |name|>
{{name}}
</PowerSelect>

Also notice you can use set inside an {{#each}} loop:

{{#each this.coreTeam as |teamMembers|}}
  <SelectTimeZone @value={{teamMember.timeZone}} @update={{set teamMember.timeZone}} />
{{/#each}}

Now this is interesting because the alternative to set is something like
@update={{this.setTimeZone teamMember}} with that action:

@action setTimeZone(member, timeZone) {
  member.timeZone = timeZone;
}

This is IMHO worse then set, because the template has about the same size, but we dont know what setTimeZone does before checking it, making it quite verbose. By using set we clearly indicate this will just set a property and do nothing else. However in opposite to a bound variable with <Input @value={{this.data}} /> it is easy to refactor if we want to do more then just to set a property.

And I think the fact alone that mut is used by a lot of people in real world applications means that we need set.

@mehulkar
Copy link
Contributor

mehulkar commented Feb 22, 2020

@luxferresum the PowerSelect example makes sense but I'm not sure it's a good reason not to use an action in the JS instead. There must already be JS in order to use this.destination. I think a better example would be:

{{#let (lookup (hash type="service" name="foo") as |someService|}}
  {{set someService (get someService "destination"))}}
{{/let}}

In other words, something that didn't previously have a Component JS class, and would be able to keep it that way. To me, that seems like an edge case (and if that example above is any indication, a bit convoluted also).

The other example with each also makes sense, but I'm not convinced that less verbosity is a good reason to introduce this into core.

That said, I still support this is as a pretty common sense replacement for mut, for all the reasons you've pointed out. I would encourage deprecating mut as part of this RFC.

@richard-viney
Copy link
Contributor

richard-viney commented Feb 22, 2020

FWIW we stopped using mut some time back and had a general rule of "templates should not contain direct mutations". We also avoid addons such as ember-ref-modifier for the same reason. Everything worked fine, though yes you did end up with some more one line actions to just set a value. I found the explicitness of this approach to be useful (less template magic), and because we use TypeScript we then also got some type checking on the assignment operation which was another benefit.

@simonihmig
Copy link
Contributor

My 2 cents:

  • I very rarely used mut, so don't really have a strong need for set
  • I would prefer the magic-less versions, i.e. {{set context "property"}} (note the similarity to {{ref context "property"}}), and drop the "2 argument" version in favor of the slightly more verbose use with {{fn}}, as {{on}} also does not allow partial application of arguments, but requires {{fn}} for this
  • I think this could (and so should) be implemented as an addon, either a simple user-land addon, or - if the community agrees this should be part of the default Ember DX - as an "official" one part of the default blueprint, just as e.g. ember-render-modifiers

@wagenet
Copy link
Member

wagenet commented Jul 23, 2022

There's some healthy discussion around this so I'd like to see if we can get a more definitive conclusion here. I'll bring this up with the rest of the core team.

@wagenet wagenet added the S-Proposed In the Proposed Stage label Dec 2, 2022
@wagenet
Copy link
Member

wagenet commented Dec 2, 2022

The magic-less version is implemented in this add-on: https://github.com/pzuraq/ember-set-helper. I too am more inclined towards a magic-less version.

@johanrd
Copy link

johanrd commented Jan 29, 2024

Here's a PR for a V2 addon version of the set helper with added types in case anyone here is using the set-helper would like to do a review of over in addon-land:

ember-helper version (<4.5 support, I guess): adopted-ember-addons/ember-set-helper#50
plain-function version (>=4.5 support): adopted-ember-addons/ember-set-helper#51

Thanks!

@cah-brian-gantzler
Copy link

With the advent of gjs/gts imports, I have found the import for the set helper conflicting with the import of set from @ember/object.

I have to do the following all the time

import { set } from '@ember/object';
import setHelper from 'ember-set-helper/helpers/set';

Depending on where you export this from, feels like a inject as service issue all over again. Dont have a suggestion, but I think set as a name will yield conflicts.

@johanrd
Copy link

johanrd commented Mar 7, 2024

@cah-brian-gantzler True. There are others as well, especially when plain-functions-as-helpers makes it more tempting to use wider-javascript-community functions directly in templates (e.g. there are a lot of function name overlaps between lodash and 'good old' ember functions).

That said, I have not had any use for import { set } from '@ember/object' when working in octane-era ember. In what use cases do you use it? (as opposed to direct assignments of @Tracked values?)

@cah-brian-gantzler
Copy link

when converting older code I do not always rewrite all sets (even if var becomes tracked). But set is sometimes more convenient.

    let propertyName = 'fieldName';
    set(this, `child.${fieldName}.someProp`, newValue);

than

    let propertyName = 'fieldName';
    this.child?.[fieldName]?.someProp = newValue;

Really depends if you have variables that hold the property names.

Set is also used a lot in tests (and I like to do the tests as gts as well) because it effectively does a notifyPropertyChange, so you dont have to create full object with @Tracked vars. Although the context change from this to scope in the rendered template changes some things.

All in all though you are correct, its use is waning. My point was its far from gone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-Proposed In the Proposed Stage T-templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.