From 7f9edd73f8a16107097d1d806753af9af6c9e2d3 Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Sat, 25 Jan 2020 09:53:45 -0800 Subject: [PATCH] [BUGFIX release] Correctly links ArrayProxy tags to `arrangedContent` 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. --- .../tests/integration/syntax/each-test.js | 21 ++++++++++++++++++- .../-internals/metal/lib/array_events.ts | 13 +++++++----- .../runtime/lib/system/array_proxy.js | 21 ++++++++++++++++++- 3 files changed, 48 insertions(+), 7 deletions(-) diff --git a/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js b/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js index ff1a9d214b0..5bc867f9f95 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js +++ b/packages/@ember/-internals/glimmer/tests/integration/syntax/each-test.js @@ -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'; @@ -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 { diff --git a/packages/@ember/-internals/metal/lib/array_events.ts b/packages/@ember/-internals/metal/lib/array_events.ts index 306a7f1af0b..f898b76c9bf 100644 --- a/packages/@ember/-internals/metal/lib/array_events.ts +++ b/packages/@ember/-internals/metal/lib/array_events.ts @@ -32,7 +32,8 @@ export function arrayContentDidChange( array: T, startIdx: number, removeAmt: number, - addAmt: number + addAmt: number, + notify = true ): T { // if no args are passed assume everything changes if (startIdx === undefined) { @@ -50,11 +51,13 @@ export function arrayContentDidChange( 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]); diff --git a/packages/@ember/-internals/runtime/lib/system/array_proxy.js b/packages/@ember/-internals/runtime/lib/system/array_proxy.js index a6514c4f78b..18810379d34 100644 --- a/packages/@ember/-internals/runtime/lib/system/array_proxy.js +++ b/packages/@ember/-internals/runtime/lib/system/array_proxy.js @@ -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', @@ -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() { @@ -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); + }, });