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.

Sign up for GitHub

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

Jump to bottom

Move away from ArrayComputed/ReduceComputed #11513

Merged
merged 2 commits into from
Jun 26, 2015
Merged

Move away from ArrayComputed/ReduceComputed #11513

merged 2 commits into from
Jun 26, 2015

Conversation

stefanpenner
Copy link
Member

@wagenet & @stefanpenner

Fixes #11181
Fixes #10343
Fixes #9739
Fixes #9462
Fixes #4919
Fixes #4231
Fixes #3706
Fixes #5596
Fixes #9485
Fixes #9492
Fixes #5319
Fixes #5268
Fixes #4831
Fixes #5558

  • almost everything
  • computed sort
  • make the new macros all readOnly
  • double check open issues
  • copious peer review

@stefanpenner stefanpenner mentioned this pull request Jun 19, 2015
Closed
2 tasks
et
et reviewed Jun 19, 2015
View reviewed changes
packages/ember-runtime/tests/computed/reduce_computed_macros_test.js
});


function commonSortTests() {
<<<<<<< HEAD
Copy link

Choose a reason for hiding this comment

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

Merge conflict?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ya, im just going through the work to re-integrate and fix this. Figured I would get it as a PR sooner rather then later so others saw this was being worked on.

@stefanpenner stefanpenner force-pushed the remove-rc-dc branch 11 times, most recently from 84bc934 to 6d7b5a5 Compare June 19, 2015 23:32
@stefanpenner stefanpenner changed the title Begin moving away from ArrayComputed Move away from ArrayComputed/ReduceComputed Jun 19, 2015
@stefanpenner stefanpenner force-pushed the remove-rc-dc branch 2 times, most recently from 0ff9e64 to 9cd39a3 Compare June 20, 2015 00:59
@stefanpenner
Copy link
Member Author

don't forget about #5150

@stefanpenner stefanpenner force-pushed the remove-rc-dc branch 3 times, most recently from 21cd882 to 764d7f9 Compare June 24, 2015 01:24
mixonic
mixonic reviewed Jun 24, 2015
View reviewed changes
packages/ember-runtime/lib/computed/reduce_computed_macros.js
// This is a bit ugly
var fullKey = dependentKey;
if (/@each/.test(fullKey)) {
dependentKey = fullKey.replace(/\.@each.*$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

doesn't this change the invalidation behavior of the macro in a very unexpected way?

Copy link
Member Author

Choose a reason for hiding this comment

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

unsure, this snippet is somewhat inherited. Got a test case in mind?

Actually I believe this is correct.

given, the DK of [email protected] we ultimately get collection
given, the DK of collection.@each.{attribute,other,other1} we ultimately get collection

The DK here is actually used to grab the collection for subsequent array based mutation.

name is likely not the best.

@mixonic
Copy link
Member

mixonic commented Jun 24, 2015

This looks pretty great :-)

@stefanpenner stefanpenner force-pushed the remove-rc-dc branch 3 times, most recently from 90ddef6 to a727222 Compare June 24, 2015 03:01
@rwjblue
Copy link
Member

rwjblue commented Jun 25, 2015

👍 - LGTM (tested in a few apps)...

@stefanpenner - Can you prefix with [BUGFIX beta]?

@raytiley
Copy link
Contributor

Hopefully addons are using ember-try :)

@rwjblue
Copy link
Member

rwjblue commented Jun 26, 2015

@stefanpenner - The last rebase introduced a failure.

@stefanpenner
Copy link
Member Author

Thanks. Ill check it out this evening

@stefanpenner stefanpenner mentioned this pull request Jun 26, 2015
Merged
2 tasks
@stefanpenner
Copy link
Member Author

@rwjblue green now. This is rather large, we can briefly mention it in the meeting today to get confirmation from the rest of the team.

I suspect the motivation is not in controversy, but maybe some more want to get the chance to try it out.

@kategengler
Copy link
Member

I think this might also cover #5558

@stefanpenner
Copy link
Member Author

@kategengler yup, it should totally fix that.

@stefanpenner
[BUGFIX beta] [fixes emberjs#11181, emberjs#10343, emberjs#9739, embe…
0dc1a6c 
…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
only invoke didChange if a node is still present.
9c52115
rwjblue added a commit that referenced this pull request Jun 26, 2015
@rwjblue
Merge pull request #11513 from stefanpenner/remove-rc-dc
47ec662 
Move away from ArrayComputed/ReduceComputed
@rwjblue rwjblue merged commit 47ec662 into emberjs:master Jun 26, 2015
@rwjblue rwjblue deleted the remove-rc-dc branch June 26, 2015 19:12
@dgeb
Copy link
Member

dgeb commented Jun 26, 2015

👍 looks good - great job, @stefanpenner!

@mmun
Copy link
Member

mmun commented Jun 26, 2015

A short explanation...

ArrayComputed is no longer necessary because of Glimmer's diffing based rendering. Previously, ArrayComputed would meticulously track changes to arrays and keep metadata of splices (push, pop, shift, unshift, etc.) as they happened. With Glimmer's diffing, we no longer do any bookkeeping, instead all work is deferred until render and done in a single, very efficient, diffing pass. In order to do this efficiently, we use a key-based algorithm (pioneered by React).

If you don't know about keys yet, that's fine. Ember has a default key implementation (relying on Ember.guidFor) that is backwards compatible with {{#each}}. Semantically, diffing and ArrayComputed are identical when all keys are unique, which is the majority of cases. When dealing with duplicate keys (which aren't supported quite yet, but soon will be) there is a hypothetical semantic difference in some cases, but it shouldn't affect any real world apps.

@anulman
Copy link

anulman commented Jun 26, 2015

👍 for better performance with fewer LOC!

@davidpett
Copy link
Contributor

Thanks for the explanation @mmun, I like it and helps me understand what's going on!

@lukemelia
Copy link
Member

Ah, how I've yearned for this day...! So good.

@cyril-sf
Copy link
Contributor

#RenderEquality what a week!

@ianstarz
Copy link

The red in this diff is incredible, so great!

@toranb
Copy link
Contributor

toranb commented Jun 29, 2015

@stefanpenner great work my friend!

@taras
Copy link
Contributor

taras commented Jun 30, 2015

move_away_from_arraycomputed_reducecomputed_by_stefanpenner_ pull_request__11513 _emberjs_ember_js
sweet!

@stefanpenner
Copy link
Member Author

@taras most of the additions are improved tests

@stefanpenner stefanpenner mentioned this pull request Jul 30, 2015
Closed
@stefanpenner stefanpenner mentioned this pull request Aug 26, 2015
Closed
@wecc wecc mentioned this pull request Sep 16, 2015
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Needs Team Discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.