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 beta] Composable Computed Properties #3696

Merged
merged 1 commit into from
Jan 7, 2014
Merged

[FEATURE beta] Composable Computed Properties #3696

merged 1 commit into from
Jan 7, 2014

Conversation

twokul
Copy link
Member

@twokul twokul commented Nov 5, 2013

This feature allows you to combine (compose) different computed
properties together. So it gives you a really nice "functional
programming" like syntax to deal with complex expressions.

Example:

var sort = Ember.computed.sort,
    union = Ember.computed.union,
    equal = Ember.computed.equal,
    mapBy = Ember.computed.mapBy,
    not = Ember.computed.not,
    compare = Ember.compare,
    and = Ember.computed.and;

var Person = Ember.Object.extend(
  napTime: and(
    equal('state', 'sleepy'),
    not('thirsty'),
    not('hungry')
  )
);

var person = Person.create({
  state: 'sleepy',
  thirsty: true,
  hungry: false
});

person.get('napTime') => false
person.set('thirsty', false)
person.get('napTime') => true

var Kitties = Ember.Object.extend(
  // starting from the inside out, this CP:
  // maps two arrays,
  // takes the union,
  // and then sorts them using Em.computed.compare
  allKitties: sort(
    union(
        mapBy('whiteKitties', 'name'),
        mapBy('blackKitties', 'name')
    ), compare
  )
);

var kitties = Kitties.create({
  whiteKitties: Ember.A([
    { name: 'whiskeys' },
    { name: 'bagels' }
  ]),
  blackKitties: Ember.A([
    { name: 'little boots' }
  ]),
});

// `allKitties` CP gets the union of both arrays
// and sorts them using Em.computed.compare
kitties.get('allKitties') => ['bagels', 'little boots', 'whiskeys']

Examples above use Ember.Object but Composable CPs work perfectly fine with POJOs:

var not = Ember.computed.not,
    obj = {
      firstName: 'Joe',
      lastName: 'Black',
      hungry: true
    };

Ember.defineProperty(obj, 'happy', not('hungry'));

Ember.get(obj, 'happy'); // => false

Kudos to brilliant @hjdivad for his help.

Update on two points mentioned below:

  • out of the box chaining syntax
  • alias/get to determine literal or bound & compatibility strategy

Both of them belong in a separate PR, so I'm going to remove them from the check list. It does seem reasonable to have something like that in ember, but I'm not sure what the best approach is to implement this kind of functionality and I don't want it to hold this PR.

@stevekane
Copy link

Honestly, this is really hard to read. I would be pretty angry if someone gave me this code. However, the intent is wonderful and I'm glad you guys are putting effort into it. I think creating a "wrapper" object of sorts (akin to _.chain from underscore/lodash) might help to separate the intent from the noise. Additionally, I would rewrite your example to use functions and vars to make it easier to grok. Your purpose is being lost in syntax and indentation.

@twokul
Copy link
Member Author

twokul commented Nov 5, 2013

@stevekane I update the example, should be better now.

@stefanpenner
Copy link
Member

currently example looks good to me. Im am insanely excited for this.

@endash
Copy link
Contributor

endash commented Nov 5, 2013

👍 would love to get rid of a bunch of props that just exist to compose

@kingpin2k
Copy link
Contributor

Agreed, after actually reading the comments this will be wicked.

@stevekane
Copy link

var sort = Ember.computed.sort,
    filter = Ember.computed.filter,
    filterBy = Ember.computed.filterBy,
    mapBy = Ember.computed.mapBy;

var people = [
  {name: "David", gender: "m", rank: 3, location: "Denver"},
  {name: "Sally", rank: 2, gender: "f", location: "Denver"},
  {name: "Franky", rank: 7, gender: "m", location: "Boston"},
  {name: "Josie", rank: 8, gender: "f", location: "Chicago"},
  {name: "Monica", rank: 1, gender: "f", location: "Denver"},
  {name: "LeftEye", rank: 4, gender: "f", location: "Denver"},
  {name: "Tboz", rank: 5, gender: "f", location: "Chicago"},
  {name: "Neo", rank: 9, gender: "m", location: "San Francisco"},
  {name: "Evangeline", rank: 6, gender: "f", location: "New York"},
  {name: "Rocky", rank: 10, gender: "m", location: "Boston"},
];

//Filter by gender
//Sort by rank
//Take the first 5 

function takeFirstFive = function (each, index) { return index < 5; }

var PeopleController  = Ember.ArrayController.extend({

    content: people,

    //inline "LISP" style -- hard to read and reason about...
    topFiveWomen: filter(sort(filterBy("gender", "f"), "rank"), takeFirstFive),

    //same as above, but "grouped" by indendation -- still hard to reason about....
    topFiveWomen: 
    filter(
        sort(
            filterBy("gender", "f"), 
            "rank"
        ), 
        takeFirstFive
    ),

    topFiveWomen: Ember.computed.chain("content", [
       filterBy("gender", "f"),
       sort("rank"),
       filter(takeFirstFive)
    ])
});

@twokul
Copy link
Member Author

twokul commented Nov 5, 2013

@stevekane maybe this is better? I wanted to get general feedback on the idea but chaining likely to be added.

 topFiveWomen: filterBy("gender", "f").sort("rank").filter(takeFirstFive)

@kingpin2k
Copy link
Contributor

The chaining is significantly easier to read.

@@ -450,6 +450,7 @@ ReduceComputedPropertyInstanceMeta.prototype = {
@constructor
*/
function ReduceComputedProperty(options) {
// TODO: ComputedProperty.apply(this, ?)
Copy link
Member

Choose a reason for hiding this comment

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

are we saving the dependent CPs? seems like we still need to do that.

@stevekane
Copy link

@twokul , the style you suggest would be nice but attaching Ember.computed methods to the return values from Ember.computed methods would be required. I don't know how much chaos that might case. Using a chain wrapper sort of prevents this dirtiness from creeping into the system.

@twokul
Copy link
Member Author

twokul commented Nov 5, 2013

@stevekane adding chaining would be easy.

@stevekane
Copy link

@twokul Either option has extreme appeal to me. I think inside-out FP is to be avoided in the "real world". It's just significantly harder to reason about "at a glance" and syntax errors are common. Thanks for doing this! Huge contributions.

@twokul
Copy link
Member Author

twokul commented Nov 5, 2013

@stevekane fair enough. noted, thanks for your feedback!

@hjdivad
Copy link
Member

hjdivad commented Nov 5, 2013

Checklist of things

  • Verify performance okay
  • Verify no mem leaks
  • Code cleanup
  • Feature flag
  • work with POJOs
  • decide out of the box chaining syntax, if any
  • Decide re: alias/get to determine literal or bound & compatibility strategy

Some notes:

  1. This PR is obviously not ready to be merged. It's a starting point for discussion. Once there's some traction we should put this behind a feature flag and also sort out some other things.
  2. It doesn't work with POJOs yet. It ought to.

We need a couple benchmarks to verify this doesn't do too much perf damage. There are two possible points of concern: the changes to Em.get and the lazy indirection. I have no reason to believe either of them are significant, but as the saying goes: "better safe, than dinner with Freys".

As @twokul noted, chaining is easy to add. It's a question of choosing the syntax. The CP API is currently very light: almost everything is private. Because of this I suspect it's not much trouble to add all the stock macros to the prototype. However, if there are concerns, adding a chain function "works", although I'd rather provide chain as a separate library than put it in stock ember. I think if ember is to provide a chain syntax out of the box it should be with the CP prototype and not an underscore style wrapper.

On The Matter of String Literals

Something that's currently very annoying about CPs is that literal vs bound must be decided at macro-design time, rather than callsite-design time. One of the nice things about this approach is it makes it very easy to fix this.

var equals = Em.computed.equals,
      // "literal"
      l = Em.computed.literal;

equals(l('Jaime Lannister'), l('Jaime Lannister'));  // "Jaime Lannister" === "Jaime Lannister"
equals('name', l('Jaime Lannister'));  // get("name") === "Jaime Lannister"
equals('name', 'otherName');  // get("name") === get("otherName")

This would basically require changing all of the current Em.computeds to use get for everything. That would break backwards compatibility. My suggestion for dealing with this is to have all the standard Em.computeds use the new approach when any of their arguments are computed properties, and the old approach otherwise, deprecating the old approach in 2.0 and eliminating it in 3.0.

@ssured
Copy link
Contributor

ssured commented Nov 5, 2013

Awesome! This cleans up a lot of intermediate properties which makes code hard to read, amazing work! Chaining of computed properties is also suggested in #3406 for implementing throttling and debouncing. Could that implementation take the same approach as described here?

@slindberg
Copy link
Contributor

👍 This would allow me to clean up so much bloat in my models, I've been planning to try and tackle it myself.

Also, IMO @hjdivad's idea to differentiate string literals from properties is important to include in any major addition to the computed property API. I can't count how many times I've wanted to use Em.computed.equals to compare the value of two properties for equality.

@mehulkar
Copy link
Contributor

mehulkar commented Nov 5, 2013

I would ❤️ chaining more than the style in the example. Also okay with

propName: Em.computed.chain('content', [
  sort('someSortingFunc'),
  filterby('someKey', value),
  mapBy('anotheKey')
]

@quantizor
Copy link

@mehulkar I like that array syntax a lot actually. Helps split out the chained pieces more visually with minimal additional overhead.

@twokul
Copy link
Member Author

twokul commented Nov 6, 2013

#3680

@stefanpenner
Copy link
Member

@mehulkar its not just chaining, is composition. The current syntax feels correct.

@trek
Copy link
Member

trek commented Nov 6, 2013

👍 nice and expressive, very little additional code.

@stefanpenner
Copy link
Member

@trek and due to @kselden and @hjdivad previous work, the internals tend to do the optimal thing. Amazing.

@jimsynz
Copy link
Contributor

jimsynz commented Nov 11, 2013

This is great work! I didn't think you'd be making enumerology obsolete so quickly :)

@twokul
Copy link
Member Author

twokul commented Nov 18, 2013

@kselden would you mind looking at benchmarks to see if they make sense or not?

@twokul
Copy link
Member Author

twokul commented Nov 18, 2013

ember benchmarks 2013-11-18 02-25-32

name: 'Alex',
state: 'happy'
});
});
Copy link
Member

Choose a reason for hiding this comment

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

This is not the comparison we want; we should be comparing object creation with CCP vs creation with explicit CPs

obj = Ember.Object.extend({
  name: 'Alex',
  state: 'happy',
  equals_state_sleepy: equals('state', 'sleepy'),
  not_equals_state_sleepy: not('equals_state_sleepy')
}).create({})

@twokul
Copy link
Member Author

twokul commented Nov 28, 2013

ember benchmarks 2013-11-27 19-15-55

@twokul
Copy link
Member Author

twokul commented Nov 30, 2013

ember benchmarks 2013-11-29 22-44-17

@hjdivad
Copy link
Member

hjdivad commented Dec 3, 2013

Notes from some benchmark observations.

  • this.constructor.reopen for type instance
  • only makeLazyFunc when necessary: doing so all the time reduces throughput by ~20%
  • drop backwards compatibility for existing macros that want to support cp args; they will still work, but will need to be updated to support cp args; drop support for Em.get(cp); update registerComputed to swap cp args for implicitCPKey and see if this is in fact responsible for the remaining ~5% reduction in throughput.

@stefanpenner
Copy link
Member

+1

@hjdivad can you c/d this one?

@stefanpenner
Copy link
Member

can you rebase?

@hjdivad I know you also spent some time working on this, do you feel comfortable with the current implementation? I would really love to see this get in.

@ghost ghost assigned hjdivad Jan 6, 2014
@hjdivad
Copy link
Member

hjdivad commented Jan 6, 2014

@stefanpenner this mostly looks good. I have a few things to add

  • minor cleanup (kill ComputedHelpers)
  • expose normalizeDependentKeys
  • expose something like Em.computed.property - the opposite of the proposed computed.literal, above
  • test user-space macros can as easily be path/literal agnostic as the builtin ones

@stefanpenner
Copy link
Member

this commit is also labeled incorrectly.

@rjackson can you provide details as to how to adjust the commit message.

@rwjblue
Copy link
Member

rwjblue commented Jan 6, 2014

It should be [FEATURE composable-computed-properties] (along with any commits after this is merged that are related to the feature, but before it hits its first beta cycle).

This feature allows you to combine (compose) different computed
properties together. So it gives you a really nice "functional
programming" like syntax to deal with complex expressions.

Example:
```javascript
var sort    = Ember.computed.sort,
    union   = Ember.computed.union,
    equal   = Ember.computed.equal,
    mapBy   = Ember.computed.mapBy,
    not     = Ember.computed.not,
    compare = Ember.compare,
    and     = Ember.computed.and;

var Person  = Ember.Object.extend(
               napTime: and(
                          equal('state', 'sleepy'),
                            not('thirsty'),
                            not('hungry')
                          )
              );

var person = Person.create({
               state: 'sleepy',
               thirsty: true,
               hungry: false
             });

var Kitties = Ember.Object.extend(
       allKitties: sort(
               union(
                 mapBy('whiteKitties', 'name'),
                 mapBy('blackKitties', 'name')
               ),
               compare
             )
      );
var kitties = Kitties.create({
        whiteKitties: Ember.A([{
                                 name: 'whiskeys'
                               }, {
                                 name: 'bagels'
                              }]),
        blackKitties: Ember.A([{ name: 'little boots' }]),
       });

person.get('napTime') => false
person.set('thirsty', false)
person.get('napTime') => true

kitties.get('allKitties') => ['bagels', 'little boots', 'whiskeys']
```

Kudos to brilliant @hjdivad for his help.
@hjdivad
Copy link
Member

hjdivad commented Jan 7, 2014

@rjackson this should be good to merge. I will write up a proposal for literal/property agnosticism and add it as a separate PR.

rwjblue added a commit that referenced this pull request Jan 7, 2014
[FEATURE beta] Composable Computed Properties
@rwjblue rwjblue merged commit 54262c7 into emberjs:master Jan 7, 2014
@twokul twokul deleted the composable-cp branch January 7, 2014 20:03
@stefanpenner stefanpenner mentioned this pull request Jan 16, 2014
8 tasks
@trek
Copy link
Member

trek commented Jan 17, 2014

@hjdivad have the uncheked TODOs been handled on this?

@twokul
Copy link
Member Author

twokul commented Jan 17, 2014

@trek I believe the unchecked ones are going to be a part of another PR.

@trek
Copy link
Member

trek commented Jan 19, 2014

@twokul excellent. I think we'll be "go" on this when the times comes.

@hjdivad
Copy link
Member

hjdivad commented Jan 20, 2014

@trek @twokul see #4185

@ebryn
Copy link
Member

ebryn commented Mar 2, 2014

Unfortunately, the core team has decided to no-go composable computed properties.

We're not ready to make the internal computed property API public yet. We've got some interesting things in the works regarding stream composition and don't want to be constrained by this API.

We'd love to see CCP be added to ember-cpm as we're sure many apps would benefit from it.

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.