Ember.Object.set no longer allows chaining

[mike-post]

Objet.set was changed in aa81146 to return the value passed into set rather than the object which is being acted upon. This broke chaining. The following code works fine in ember 1.13.8 / ember-data 1.13.9, but throws an exception in ember 2.0 / data 2.0.0-beta.1:

      model.set('excluded', !model.get('excluded')).save().catch(reason => {
        this.set('lastError', { id: model.id, error: reason.message });
      });

The exception is:

Uncaught TypeError: model.set(...).save is not a function
toggleIgnore @ thing.js:44
_emberViewsViewsView.default.extend.send @ ember.debug.js:40885
runRegisteredAction @ ember.debug.js:22064
Backburner.run @ ember.debug.js:284
run @ ember.debug.js:19036
ActionHelper.registerAction.actions.push.handler @ ember.debug.js:22057
(anonymous function) @ ember.debug.js:39893j
Query.event.dispatch @ jquery.js:4435j
Query.event.add.elemData.handle @ jquery.js:4121

If this was going to change in 2.0.0, I would have expected a deprecation.

[stefanpenner]

If this was going to change in 2.0.0, I would have expected a deprecation.

Unfortunately this doesn't have any obvious deprecation. Maybe returning an ES6 proxy? But that sounded like a massive can of worms.

But It should have been listed somewhere, @cibernox do you remember where this was documented?

@mixonic this may need to be an amendment to the 2.0 blogpost.

It is worth noting, this is to align ember get/set with ES5 getters/setters. WIthout this change, there is no hope of migrating seamlessly to them.

[mike-post]

It looks like it was intended to be documented, but it never happened: emberjs/website#2212

[stefanpenner]

It looks like it was intended to be documented, but it never happened: emberjs/website#2212

Thanks for catching this - looks like this slipped through the cracks.

[cibernox]

I can create a PR updating the deprecations guide if nobody is already on it.

[stefanpenner]

I can create a PR updating the deprecations guide if nobody is already on it.

that would be great, (i don't believe anyone else is on it yet)

[tomdale]

If this is breaking apps in the wild, I think we should revert the change. The benefits are just not worth the pain.

[wycats]

@tomdale I agree.

[cibernox]

what other options do we have to end bet aligned with regular setters in a non breaking way?
Advertise this as the only breaking change in the release notes is not explicit enough?

[tomdale]

@cibernox "Aligning with regular setters" is not a strong enough benefit to end users to justify breaking things. The 2.0 message—1.13 apps without deprecations work—is too strong to sacrifice it for a very, very marginal improvement.

[stefanpenner]

There is also the confusion with super and cp setter to take into account. @cibernox mind sharing the example, as it will help enumerate the rationale

@tomdale / @wycats this was discussed and explained in a meeting where you both acknowledged these caveats and confirmed it was acceptable.

If you want to backtrack that's fine. But that means there is no way to ever deprecate this functionality.

Also note, this is less breaking then things deemed acceptable between 1.12 and 1.13. I also suspect there are more relevant differences in 2. I would hate for this to set a precedence.

This will also break apps who during beta who began to rely on the JS aligned semantics.

[cibernox]

I think the main problem here is that we missed this in the release notes. I think if advertised people would understand that only one breaking change that can't be done otherwise is acceptable in a major version.

I am ok with reverting the change if that saves real pain in the wild (it a very very unused feature tho), but I don't see a way of ever do that in a deprecatable way.

[cibernox]

@stefanpenner Not sure about what example you mean. The one that comes to my mind with CPs is that people keeps being bitted by the fact that computed properties cache the return value and i've seen many people doing this by mistake, and caching this. I've seen this tens of times.

computed('other', {
  get() { return this.get('other'); },
  set(_, v) { 
    // some code ...
    return this.set('other', v); 
   }
})

[stefanpenner]

There is a possibility that we will revert this, in-favor of the fix landing in 3.0. When we hope there will be sufficient community mass on ember-cli enalbing static analysis based deprecation that will be able to cover sufficient use-cases.

I still would prefer to see this not to change now, but I can accept ^.

The major concern is potentially community pain.

Whatever choice is made it must happen in the next 48h

[cibernox]

In the meanwhile, this is an example of the confusing use case that confuses devs using super in computed properties:

var TwitterUser = Ember.Object.extend({
  handler: 'Tomster',

  aka: Ember.computed('handler', {
    get() {
      return this.get('handler');
    },
    set(_, v) {
      return this.set('handler', v);
    }
  })
});

var TwitterTroll = TwitterUser.extend({
  aka: Ember.computed('handler', {
    get() {
      return this.get('handler') + ' LOL';
    },
    set(k, v) {
      return this._super(...arguments);
    }
  })  
});

var troll = TwitterTroll.create();

troll.get('aka'); // "Tomster"

troll.set('aka', 'HorseJS'); // In ember 1.X this returns the `troll` instance, while in Ember 2.0 it returns "HorseJS"

troll.get('aka'); // Also, since the return value of the call to `_super` has been cached, 
                  // in Ember 1.X this has cached the `troll` instance, while in ember 2.0 returns "HorseJS"

[oriSomething]

maybe there is a way to deprecated it. as long as it was the new computed function that was used with get and set, only if the set property returns undefined you'll get the chaining effect and a deprecation warning, but when you return a value, you'll get the new effect. it's not 100% cover, but in most cases my guess it could go well.
i remember in the old computed the return value of set was cached, but it doesn't seem to work with the new one

[cibernox]

@oriSomething the caching policy of CP setters hasn't changed AFAIK.

However the linting mechanism that @mmun is proposing may catch almost all usages.

[markoftheweb]

This has had an impact on the tutorial I just went through:
https://guides.emberjs.com/v2.4.0/tutorial/autocomplete-component/
https://github.com/emberjs/guides/blob/master/source/localizable/tutorial/autocomplete-component.md

Where
this.set('filteredList').clear();
and
this.set('model').clear();

The setter seems to want to return the value

[mmun]

@markoftheweb We should change that guide to make it clearer. There's no need to use clever chaining (whether it works or not) in a guide.

[cibernox]

@markoftheweb Those lines look like a typo to me. Set requires two arguments. Whoever wrote that wanted to write this.get('filteredList').clear();