rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

set helper

Open luxzeitlos opened this issue 4 years ago • 14 comments

Rendered

luxzeitlos avatar Feb 20 '20 19:02 luxzeitlos

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

btecu avatar Feb 20 '20 23:02 btecu

@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.

pzuraq avatar Feb 20 '20 23:02 pzuraq

@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.

luxzeitlos avatar Feb 21 '20 20:02 luxzeitlos

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.

mehulkar avatar Feb 21 '20 23:02 mehulkar

@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.

luxzeitlos avatar Feb 22 '20 00:02 luxzeitlos

@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.

mehulkar avatar Feb 22 '20 00:02 mehulkar

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.

richard-viney avatar Feb 22 '20 02:02 richard-viney

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

simonihmig avatar Feb 22 '20 17:02 simonihmig

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 avatar Jul 23 '22 17:07 wagenet

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.

wagenet avatar Dec 02 '22 15:12 wagenet

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): https://github.com/pzuraq/ember-set-helper/pull/50 plain-function version (>=4.5 support): https://github.com/pzuraq/ember-set-helper/pull/51

Thanks!

johanrd avatar Jan 29 '24 08:01 johanrd

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.

cah-brian-gantzler avatar Mar 06 '24 15:03 cah-brian-gantzler

@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?)

johanrd avatar Mar 07 '24 19:03 johanrd

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.

cah-brian-gantzler avatar Mar 07 '24 19:03 cah-brian-gantzler