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

[BUGFIX beta] fix for lastObject/firstObject update issue #15510

Merged
merged 1 commit into from
Jul 18, 2017
Merged

[BUGFIX beta] fix for lastObject/firstObject update issue #15510

merged 1 commit into from
Jul 18, 2017

Conversation

bekzod
Copy link
Contributor

@bekzod bekzod commented Jul 16, 2017

enhanced #15110 PR, @stefanpenner

stefanpenner
stefanpenner requested changes Jul 16, 2017
View reviewed changes
packages/ember-runtime/lib/mixins/array.js Outdated
propertyWillChange(array, 'lastObject', meta);
propertyDidChange(array, 'lastObject', meta);
if (cache !== undefined) {
if (cache.firstObject !== undefined && startIdx === 0 && objectAt(array, 0) !== cacheFor.get(cache, 'firstObject')) {
Copy link
Member

Choose a reason for hiding this comment

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

i think one of the goals @hjdivad had, was to avoid the objectAt(x) lookups, as they would force the CP to be accessed.

I'll ping @hjdivad to chime in.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah but you need to invalidate cp when it changes no ?

Copy link
Member

Choose a reason for hiding this comment

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

If the index for firstObject / lastObject changed, then we should broadcast. Even if the object is equal. That means we wont need both the objectAt stuff and the range based detection.

@bekzod
Copy link
Contributor Author

bekzod commented Jul 17, 2017

I updated my PR, so it does not call objectAt but now behavior is changed of firstObject/lastObject change event, so now it notifies change even if same object as firstObject/lastObject is shifted/pushed to array , I think it is still better than how was before.
adjusted/added test accordingly @stefanpenner

@bekzod bekzod force-pushed the array-first-last-object branch 2 times, most recently from 2754363 to b036634 Compare July 17, 2017 14:40
@stefanpenner
Copy link
Member

@bekzod ya, that was the approach @hjdivad was taking. It is likely a trade-off we are good with.

hjdivad
hjdivad suggested changes Jul 17, 2017
View reviewed changes
packages/ember-runtime/lib/mixins/array.js Outdated
}
if (cache !== undefined) {
if (cache.firstObject !== undefined && startIdx === 0) {
propertyWillChange(array, 'firstObject');
Copy link
Member

Choose a reason for hiding this comment

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

This PR drops && (addAmt > 0 || removeAmt > 0) because we can safely assume one of those must be > 0?

Copy link
Member

Choose a reason for hiding this comment

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

(which is fine, just want to confirm)

packages/ember-runtime/lib/mixins/array.js Outdated

let lastIndex = length - delta - 1;
let lastAffectedIndex = startIdx + removedAmount;
if (lastIndex < lastAffectedIndex) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this will miss cases where lastObject changes from negative indices, eg

[0, 1, 2, 3].replace(-2, 2);
// => [0, 1]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, was not sure if negative startIndex could be passed will handle that

Copy link
Member

Choose a reason for hiding this comment

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

awesome thanks @bekzod 👍

packages/ember-runtime/tests/suites/mutable_array/pushObject.js
@@ -54,4 +54,26 @@ suite.test('[A,B,C].pushObject(X) => [A,B,C,X] + notify', function() {
equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject');
});

suite.test('[A,B,C,C].pushObject(A) => [A,B,C,C] + notify', function() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

We'll need a test for removing the tail from a negative index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added that, you could take another shot when you have time, thanks

@hjdivad hjdivad mentioned this pull request Jul 17, 2017
Closed
hjdivad
hjdivad approved these changes Jul 17, 2017
View reviewed changes
Copy link
Member

@hjdivad hjdivad left a comment

Choose a reason for hiding this comment

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

LGTM

packages/ember-runtime/tests/suites/mutable_array/replace.js
@@ -126,6 +126,27 @@ suite.test('[A,B,C,D].replace(2,2) => [A,B] + notify', function() {
equal(observer.validate('firstObject'), false, 'should NOT have notified firstObject once');
});

suite.test('[A,B,C,D].replace(-1,1) => [A,B,C] + notify', function() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

packages/ember-runtime/lib/mixins/array.js
let delta = addedAmount - removedAmount;
let previousLength = length - delta;

let normalStartIdx = startIdx < 0 ? previousLength + startIdx : startIdx;
Copy link
Member

Choose a reason for hiding this comment

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

👍

@stefanpenner
Copy link
Member

@krisselden bugfix beta or let it ride the canary train?

@stefanpenner
Copy link
Member

chatted with @krisselden hes ok with beta

@stefanpenner
Copy link
Member

@bekzod can you squash, and prefix with [BUGFIX beta] ?

@bekzod bekzod changed the title fix for lastObject/firstObject update issue [BUGFIX beta] fix for lastObject/firstObject update issue Jul 18, 2017
@bekzod
Copy link
Contributor Author

bekzod commented Jul 18, 2017

@stefanpenner done

@locks locks merged commit 23a8c92 into emberjs:master Jul 18, 2017
@bekzod bekzod deleted the array-first-last-object branch July 18, 2017 06:22
@stefanpenner stefanpenner mentioned this pull request Jul 19, 2017
Closed
@bekzod bekzod mentioned this pull request Mar 25, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@krisselden krisselden krisselden approved these changes

@hjdivad hjdivad hjdivad approved these changes

@stefanpenner stefanpenner stefanpenner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@bekzod @stefanpenner @hjdivad @krisselden @locks