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

[FEATURE computed-property-syntax-new] #9477

Closed
wants to merge 2 commits into from
Closed

[FEATURE computed-property-syntax-new] #9477

wants to merge 2 commits into from

Conversation

mike-north
Copy link
Contributor

RFC: emberjs/rfcs#11

This feature adds some optional new syntax for defining computed properties. Current methods for building computed properties still work fine, but now we can pass an object instead of a function

a: Ember.computed('b', 'c', {
  get: function () { ... },
  set: function (key, val, oldVal) { ... }
});

So, here's a concrete example of a "writable" macro with the current syntax

var Person = Ember.Object.extend({
  fullName: Ember.computed('firstName', 'lastName', function (key, val, oldVal) {
    if (arguments.length === 1) {
      //getter
      return this.get('firstName') + ' ' + this.get('lastName');
    }
    else {
      //setter
      var nameComponents = val.split(' ');
      this.setProperties({
        firstName: nameComponents[0],
        lastName: nameComponents[1]
      });
      return val;
    }
  })
});

And the equivalent with the proposed syntax.

var Person = Ember.Object.extend({
  fullName: Ember.computed('firstName', 'lastName', {
    get: function () { // Getter
      return this.get('firstName') + ' ' + this.get('lastName');
    },
    set: function (key, val, oldVal) { // Setter
      var nameComponents = val.split(' ');
      this.setProperties({
        firstName: nameComponents[0],
        lastName: nameComponents[1]
      });
      return val;
    }
  })
});

test('writable - setting property invokes "set" fn', function () {
o.set('half', 11);
equal(o.get('a'), 5.5, 'updates dependant properties #1');
equal(o.get('b'), 5.5, 'updates dependant properties #2');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note to self:

var vals = o.getProperties(['a', 'b']);
deepEqual(vals, {a: 5.5, b: 5.5}, 'Setting computed property updates dependent properties as expected');

@workmanw
Copy link

workmanw commented Nov 3, 2014

I'm sorry I missed the RFC (emberjs/rfcs#11). I don't know if it's too late to provide feedback or where I should put it, but one thing that occurs to me is that often times my CPs that have complicated logic look like this:

Ember.computed('a', 'b', 'c', 'd', function (key, val) {
  if (arguments.length === 2) {
    // COMPLICATED "SET" LOGIC
  }
  // RETURN COMPLICATED "GET" LOGIC
})

In this case I'm effectively doing a "set", followed by a "get", there isn't a distinct code path for building the return value that's based on the type of operation that's taking place. I can just imagine a situation with computed-property-syntax-new where some amount of complicated logic that is necessary to build a return value is duplicated between getters and setters.

I almost wonder if set should not return a value and instead after set returns, get will be called to build a value. Or perhaps if set does return a value it is used, and if it doesn't returned a value (undefined) then get is subsequently called.

@stefanpenner
Copy link
Member

oh man, awesome! I will find some time today to review.

@cibernox
Copy link
Contributor

cibernox commented Nov 3, 2014

We stepped into each other!!

I came up with this simple solution a moment ago: #9478

@stefanpenner
Copy link
Member

We need to ensure super works for get and set correctly.

I have some internals ideas I will provide once I can review this

@mike-north
Copy link
Contributor Author

@stefanpenner looking forward to exchanging ideas. I'll add a test coverage for calling super on get and set

@knownasilya
Copy link
Contributor

👍 Really like this new syntax, can't wait for it to land.

@stefanpenner
Copy link
Member

sorry for letting this linger, I have assigned myself to review.

Can you also rebase?

@stefanpenner stefanpenner self-assigned this Dec 5, 2014
@cibernox
Copy link
Contributor

cibernox commented Dec 5, 2014

@stefanpenner PR #9527 is a duplicated of this one. Check it too.

@bcardarella
Copy link
Contributor

@stefanpenner can this be closed considering #9527 was merged?

@rwjblue
Copy link
Member

rwjblue commented Jan 15, 2015

Yes, I believe so.

@rwjblue rwjblue closed this Jan 15, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants