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

RFC: Improved CP Syntax #11

Merged
merged 5 commits into from
Nov 2, 2014
Merged

Conversation

stefanpenner
Copy link
Member

No description provided.

@stefanpenner stefanpenner changed the title initial draft RFC: Improved CP Syntax Oct 31, 2014
@jayphelps
Copy link

No mention of Function.prototype.property; is that suggesting it would no longer be the idiomatic way? (Not suggesting for/against, just clarifying)

@stefanpenner
Copy link
Member Author

@jayphelps I suspect it will continue to work for the getter-only and explicitly writable() case

I suppose the explicitly writable case without a pojo, just a FN, on set, should check arity and either call FN with 2 arguments (like today's setter CP) or stomp the CP. This should be handled by my compatibility mode and I suspect this will be deprecated for 2.0


# Motivation

Today, the setter variant of CP's is both confusing, and looks scary as sinn.
Copy link
Member

Choose a reason for hiding this comment

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

s/sinn/sin/

@rwjblue
Copy link
Member

rwjblue commented Oct 31, 2014

Very happy about this proposal. 👍

@stefanpenner stefanpenner force-pushed the cp-improved branch 3 times, most recently from 49a6001 to 3ff96da Compare October 31, 2014 14:43
tomdale added a commit that referenced this pull request Nov 2, 2014
@tomdale tomdale merged commit d75bdff into emberjs:master Nov 2, 2014
@stefanpenner stefanpenner deleted the cp-improved branch November 2, 2014 18:36
@cibernox
Copy link
Contributor

cibernox commented Nov 2, 2014

I don't know how I missed this.

There is something I can do to help from ember-cpm? Maybe betatest this syntax now?
It comes to my mind something like EmberCPM.computed:

function EmberCPM_computed(){
  var args = Array.prototype.slice(arguments, 0, -2);
  var opts = Array.prototype.slice(arguments.length - 1)

  args.push(function(){
    if (arguments.length > 1){
      opts.set.apply(this, arguments);
    } else {
      opts.get.apply(this, arguments);
    }
  });
  return Ember.computed(this, args)
}

@stefanpenner
Copy link
Member Author

Honestly you could just pr it to ember :) let iterate in a pr and release as soon as it's good :)

@mike-north
Copy link
Contributor

I've got a PR in the works. Should be opening it in a couple of hours

@mike-north
Copy link
Contributor

Ok, I've got something working now :)

One part of the RFC that's going to cause some problems for people (myself included)

  • we should keep Ember.computed(fn); as shorthand for getter only

I take this to mean that if I'd written something like this today (which results in a writable computed property)

var MyType = Ember.Object.extend({
  a: 6,
  b: 12,
  half: Ember.computed('a', 'b', function (key, val, oldVal) {
    if (arguments.length < 2) {
       return 0.5*(this.get('a') + this.get('b'));
    }
    else {
      this.setProperties({
        a: val/2,
        b: val/2
      });
      return val; 
    }
  })
});

Once this proposed improvement is made, the function (3rd argument passed to Ember.computed) would not be called for "set" operations anymore. At least in my ember apps, this will be a breaking change in a huge way.

Is there any reason we can't continue to support both?

if a function is passed as the last argument to Ember.computed
   call the function for both get and set, as we do today
else if an object is passed as the last argument to Ember.computed
   call obj.get() to read (calculate) the property, and
   call obj.set() to write to the property

@knownasilya
Copy link
Contributor

@igorT This could be a good replacement for DS.Transform, which feels unnecessary..

@lukemelia
Copy link
Member

@truenorth agreed re: not breaking the existing setter approach. deprecating it would reasonable/helpful if possible.

@amiel
Copy link

amiel commented Nov 4, 2014

Yes please. I love this idea!!

@cowboy
Copy link

cowboy commented Dec 9, 2014

I'd much rather see this, a la ember-auto, because as it currently stands, it's very repetitive to specify the dependent properties both in the call to Ember.computed as well as in code inside the getter.

fullName: Ember.computed('firstName', 'lastName', {
  get: function(firstName, lastName) {
    return firstName + ' ' + lastName;
  },
  // ...
});

@jayphelps
Copy link

Dogpile! https://github.com/jayphelps/ember-computed-smart-property

fullName: Ember.computed.smartProperty({
  get: function () {
    return this.get('firstName') + ' ' + this.get('lastName');
  }
})

DOGPILE!!

@cowboy
Copy link

cowboy commented Dec 9, 2014

To support this functionality with less code for properties that are only getters and not setters, but in a backwards-compatible way, maybe an argument could be added to the Function.prototype.property method to enable my requested behavior:

fullName: function(firstName, lastName) {
  return firstName + ' ' + lastName;
}.property('firstName', 'lastName', {args: true}),

Or even a whole new Function.prototype.properties method could be added that just does this by default (with the existing Function.prototype.property deprecated in favor of the improved get/set syntax)

fullName: function(firstName, lastName) {
  return firstName + ' ' + lastName;
}.properties('firstName', 'lastName'),

Or the way ember-computed-smart-property does it, which is also awesome!

@stefanpenner
Copy link
Member Author

fullName: function(firstName, lastName) {
  return firstName + ' ' + lastName;
}.properties('firstName', 'lastName'),

is interesting when a 1:1 between DK and inputs exists forsure.

This type of CP macro would make a great community add-on. The specific RFC existed to primarily remove the arity check from the current CP's as that is obviously mega terrible.

@awj
Copy link

awj commented Apr 7, 2015

I like the general idea, but am concerned about it creating such a difference between Ember.computed and Function.property. Right now they are explained as being essentially equivalent, and the guide covers writeable computed properties using Function.property.

I'm not sure how Function.property could work in the proposed rfc aside from mapping the same function into get and set in the new syntax.

That concern aside, 👍

@cibernox
Copy link
Contributor

cibernox commented Apr 7, 2015

@awj The Function#property is just another way to write a getter-only Ember.computed.
One of the reasons behind this RFC was to explicitly disallow having the same function as getter and setter (makes ember internals simpler too) and encourage people to not use the prototype extensions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.