Skip to content

Commit

Permalink
[BUGFIX release] Correctly links ArrayProxy tags to arrangedContent
Browse files Browse the repository at this point in the history
Currently, if `arrangedContent` is overridden in an ArrayProxy with a
computed property that depends on changes to another array/context,
those changes will not propagate correctly. This is because we never
link the tags of the ArrayProxy to the corresponding tags of the
`arrangedContent`, instead relying on array observers to propagate
changes. This works when the underlying array is being changed directly,
but _doesn't_ work if the array is being replaced entirely (e.g. the
computed property has invalidated and needs to recompute).

This PR ensures that ArrayProxy tags are setup correctly, so that if
`arrangedContent` ever changes, the proxy will also propagate those
changes. This will affect anything that depends on the ArrayProxy
directly, such as `{{#each}}` loops and other computed properties.

One side effect of this is that ArrayProxy's no longer need to manually
dirty themselves, and in fact attempting to do so can trigger the
backtracking rerender assertion (specifically when the proxy first
attempts to update/synchronize while rendering). Internally, a boolean
flag has been added to the array change methods to allow it to opt-out
of sending a notification.
  • Loading branch information
Chris Garrett committed Feb 1, 2020
1 parent b6ae16d commit 7f9edd7
Show file tree
Hide file tree
Showing 3 changed files with 48 additions and 7 deletions.
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { moduleFor, RenderingTestCase, applyMixins, strip, runTask } from 'internal-test-helpers';

import { get, set, notifyPropertyChange } from '@ember/-internals/metal';
import { get, set, notifyPropertyChange, computed } from '@ember/-internals/metal';
import { A as emberA, ArrayProxy, RSVP } from '@ember/-internals/runtime';
import { HAS_NATIVE_SYMBOL } from '@ember/-internals/utils';

Expand Down Expand Up @@ -1089,6 +1089,25 @@ moduleFor(
}
);

moduleFor(
'Syntax test: {{#each}} with array proxies, arrangedContent depends on external content',
class extends EachTest {
createList(items) {
let wrapped = emberA(items);
let proxy = ArrayProxy.extend({
arrangedContent: computed('wrappedItems.[]', function() {
// Slice the items to ensure that updates must be propogated
return this.wrappedItems.slice();
}),
}).create({
wrappedItems: wrapped,
});

return { list: proxy, delegate: wrapped };
}
}
);

moduleFor(
'Syntax test: {{#each as}} undefined path',
class extends RenderingTestCase {
Expand Down
13 changes: 8 additions & 5 deletions packages/@ember/-internals/metal/lib/array_events.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ export function arrayContentDidChange<T extends { length: number }>(
array: T,
startIdx: number,
removeAmt: number,
addAmt: number
addAmt: number,
notify = true
): T {
// if no args are passed assume everything changes
if (startIdx === undefined) {
Expand All @@ -50,11 +51,13 @@ export function arrayContentDidChange<T extends { length: number }>(

let meta = peekMeta(array);

if (addAmt < 0 || removeAmt < 0 || addAmt - removeAmt !== 0) {
notifyPropertyChange(array, 'length', meta);
}
if (notify) {
if (addAmt < 0 || removeAmt < 0 || addAmt - removeAmt !== 0) {
notifyPropertyChange(array, 'length', meta);
}

notifyPropertyChange(array, '[]', meta);
notifyPropertyChange(array, '[]', meta);
}

sendEvent(array, '@array:change', [array, startIdx, removeAmt, addAmt]);

Expand Down
21 changes: 20 additions & 1 deletion packages/@ember/-internals/runtime/lib/system/array_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,14 @@ import {
removeArrayObserver,
replace,
getChainTagsForKey,
tagForProperty,
arrayContentDidChange,
arrayContentWillChange,
} from '@ember/-internals/metal';
import EmberObject from './object';
import { isArray, MutableArray } from '../mixins/array';
import { assert } from '@ember/debug';
import { combine, validate, value } from '@glimmer/reference';
import { combine, update, validate, value } from '@glimmer/reference';

const ARRAY_OBSERVER_MAPPING = {
willChange: '_arrangedContentArrayWillChange',
Expand Down Expand Up @@ -109,6 +112,12 @@ export default class ArrayProxy extends EmberObject {
this._arrangedContentRevision = value(this._arrangedContentTag);

this._addArrangedContentArrayObserver();

update(tagForProperty(this, '[]'), combine(getChainTagsForKey(this, 'arrangedContent.[]')));
update(
tagForProperty(this, 'length'),
combine(getChainTagsForKey(this, 'arrangedContent.length'))
);
}

willDestroy() {
Expand Down Expand Up @@ -316,4 +325,14 @@ ArrayProxy.reopen(MutableArray, {
@public
*/
arrangedContent: alias('content'),

// Array proxies don't need to notify when they change since their `[]` tag is
// already dependent on the `[]` tag of `arrangedContent`
arrayContentWillChange(startIdx, removeAmt, addAmt) {
return arrayContentWillChange(this, startIdx, removeAmt, addAmt, false);
},

arrayContentDidChange(startIdx, removeAmt, addAmt) {
return arrayContentDidChange(this, startIdx, removeAmt, addAmt, false);
},
});

0 comments on commit 7f9edd7

Please sign in to comment.