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

fix reduce computed #9492

Closed
wants to merge 6 commits into from
Closed

fix reduce computed #9492

wants to merge 6 commits into from

Conversation

patricklx
Copy link
Contributor

This fixes computed properties that depend on other computed properties.
issue: #4941
Maybe would fix other issues.
Or introduce more... didnt run the tests yet.

From what i saw through debugging is that cacheSet only works if propertyDidChange is not called. It will just be set to undefined when calling propertyDidChange

also fixes: #9313
http://emberjs.jsbin.com/hotilafamu/1/edit?html,css,js,console,output

I've made a gist with the fixed code, it can be included in jsbins to test:
https://rawgit.com/patricklx/641946978a1e3bac833d/raw/6f0d5e84db8a5b04939b30350e211e071a378485/reduced-computed-fix.js

currenly there are some problems related to changeMeta

This fixes computed properties that depend on other computed properties.
Maybe would fix other issues
This is now required in ReduceComputedProperty. Since its not always recomputed on each change.
No need do be here anymore
@wagenet
Copy link
Member

wagenet commented Nov 4, 2014

See also #9462

@wagenet wagenet mentioned this pull request Nov 4, 2014
7 tasks
@patricklx
Copy link
Contributor Author

Another issue that would be partially fixed: #3706
http://jsbin.com/zehalemoha/1/edit?html,js,console,output
I'm not sure if this PR would fix the sortedMixin, does it use computed.sort? It doesn't look like it.

But the issue could also be fixed by returning undefined when the value is NaN
http://jsbin.com/xilezoziva/1/edit?js,output

This is caused by Ember.compare returning 0 when comparing any value with NaN

@patricklx
Copy link
Contributor Author

Checking the test cases:
for example: https://github.com/emberjs/ember.js/blob/master/packages/ember-runtime/tests/computed/reduce_computed_test.js#L614

From what i read here: http://emberjs.com/api/#method_reduceComputed
It is the correct behavior to recompute reduce computed when the callback functions return undefined , which happens in the testcase.

This is now done with this PR, but not expected in the test.
I adapt the testcase, so that addedItem and removedItem return the array.

@wagenet wagenet added the Bug label Nov 17, 2014
stefanpenner added a commit that referenced this pull request Jun 23, 2015
#9492, #5319, #5268, #4831] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit to stefanpenner/ember.js that referenced this pull request Jun 26, 2015
…rjs#9462, emberjs#4919, emberjs#4231, emberjs#3706, emberjs#5596, emberjs#9485, emberjs#9492, emberjs#5319, emberjs#5268, emberjs#4831, emberjs#5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner
stefanpenner added a commit that referenced this pull request Jun 28, 2015
#5596, #9485, #9492, #5319, #5268, #4831, #5558] Move away from AC/RC instead use the simpler naive enumerable methods, and rely on glimmers stable rendering for efficiency.

For more complex scenarios, custom solutions should be used.

@wagenet & @stefanpenner

(cherry picked from commit 0dc1a6c)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reduceComputed recomputes everything on a single change of a dependant property
2 participants