Skip to content

reduceComputed dependent keys may refer to @self.#3365

Merged
wagenet merged 2 commits intoemberjs:masterfrom
hjdivad:reduce-computed-self
Sep 10, 2013
Merged

reduceComputed dependent keys may refer to @self.#3365
wagenet merged 2 commits intoemberjs:masterfrom
hjdivad:reduce-computed-self

Conversation

@hjdivad
Copy link
Copy Markdown
Member

@hjdivad hjdivad commented Sep 9, 2013

This is mostly useful for array proxies, and in particular array
controllers that use item controllers. This allows users to, for
instance, sort items according to properties specified on array
controllers.

@self is implemented only in reduceComputed because it doesn't really make
sense for non-reduceComputed CPs.

@tchak does this PR resolve your use case?

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 9, 2013

This feature does not directly resolve #2717 but it seems reasonable to close that issue after this PR is merged if we plan to deprecate Ember.SortedMixin in favour of Ember.computed.sort.

@wagenet @joefiorini @drogus @sly7-7 thoughts?

@tchak
Copy link
Copy Markdown
Member

tchak commented Sep 9, 2013

👍 ❤️

@sly7-7
Copy link
Copy Markdown
Contributor

sly7-7 commented Sep 9, 2013

I agree concerning #2717, looking at the tests, it seems like it's a good workaround for now. And I'm 👍 if we could remove the SortableMixin :)

@tchak
Copy link
Copy Markdown
Member

tchak commented Sep 9, 2013

@hjdivad I am wondering if at some point we could build a mixins system based on your work? For ArrayControllers Mixins approach is more elegant I think.

@tchak
Copy link
Copy Markdown
Member

tchak commented Sep 9, 2013

My main concern is that everywhere I was using this.controllerFor('passengers') I have to use this.controllerFor('passengers').get('arranged') now (where arranged is the final property in a chain of sorting/filtering). This is bad for refactoring.

@lukemelia
Copy link
Copy Markdown
Member

Maybe the current SortableMixin could be refactored to be sugar for reduce CPs?

Sent from Mailbox for iPhone

On Mon, Sep 9, 2013 at 8:58 AM, Paul Chavard notifications@github.com
wrote:

My main concern is that everywhere I was using this.controllerFor('passengers') I have to use this.controllerFor('passengers').get('arranged') now (where arranged is the final property in a chain of sorting/filtering). This is bad for refactoring.

Reply to this email directly or view it on GitHub:
#3365 (comment)

@wagenet
Copy link
Copy Markdown
Member

wagenet commented Sep 9, 2013

I think we'd do what @lukemelia is suggesting. We can't drop the existing functionality, but we can change the internals.

@tchak
Copy link
Copy Markdown
Member

tchak commented Sep 9, 2013

@wagenet I think it is not only about can we drop but also about should we drop

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 9, 2013

It seems like it may be feasible to have SortableMixin define arrangedContent as a computed property created via Em.computed.sort. It's at least not immediately obvious to me why this wouldn't work.

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 9, 2013

SortableMixin = Em.Mixin.create({
  arrangedContent: Em.computed.sort('content', 'sortProperties')
})

I am not certain either way. I don't have time to look into it right now, but you could try the above and it might actually work.

@wagenet
Copy link
Copy Markdown
Member

wagenet commented Sep 10, 2013

I'd love for someone to investigate this. It would be great to make SortableMixin that small :)

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 10, 2013

SortableMixin = Em.Mixin.create({
  arrangedContent: Em.computed.sort('@self', 'sortProperties')
})

@self not 'content'. My bad.

@self.

This is mostly useful for array proxies, and in particular array controllers
that use item controllers.  This allows users to, for instance, sort items
according to properties specified on array controllers.

@self is implemented only in reduceComputed because it doesn't really make sense
for non-reduceComputed CPs.

Thanks to @tchak for pointing out the need for this.
@wagenet
Copy link
Copy Markdown
Member

wagenet commented Sep 10, 2013

This looks good to me. Need to double check the flagging setup before I merge.

wagenet added a commit that referenced this pull request Sep 10, 2013
reduceComputed dependent keys may refer to @self.
@wagenet wagenet merged commit 2f38f5d into emberjs:master Sep 10, 2013
@tchak
Copy link
Copy Markdown
Member

tchak commented Sep 11, 2013

I suppose I would be +1 on @this

The idea of just omitting first argument to refer to this did not worked out?

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 11, 2013

@tchak we could separately have reduceComputed treat no dependent arguments as @this, but it would prevent cases where you want two arrays, as in union or setDiff (or a hypothetical concat).

Having no dependent args be equivalent to @this would work. It would make the multi-array case less obvious, but it's probably a much more common case.

@sandstrom
Copy link
Copy Markdown
Contributor

I've tried to use this to exclude newly created models from a list, but it seems like this computed property fires before isNew has changed. This isn't immediately related to '@self' as a magic keyword, but related since it's a use-case.

persisted: Em.computed.filter('@self', function(email) {
  debug('computed filter run'); // fires when 'isNew' is still true
  !email.get('isNew') && !email.get('isDeleted');
})

What are your thoughts on this?

@hjdivad
Copy link
Copy Markdown
Member Author

hjdivad commented Sep 12, 2013

@sandstrom jsfiddle?

@sandstrom
Copy link
Copy Markdown
Contributor

(Here is a partially working fiddle, perhaps someone can spot the mistake I did when porting my code into a fiddle, because the existing cat should show in both lists, but new cats only in the non-filtered one).

http://jsfiddle.net/C2zTh/3/

@sly7-7
Copy link
Copy Markdown
Contributor

sly7-7 commented Sep 12, 2013

@sandstrom I'm not sure if I understand well this PR after all, but i think in your case you must use 'content', not @self (because there are no itemController. see http://jsfiddle.net/C2zTh/2/

@hjdivad hjdivad deleted the reduce-computed-self branch September 24, 2013 17:15
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.

7 participants