Skip to content

Commit

Permalink
[BUGFIX release] Fixes ArrayProxy content updates
Browse files Browse the repository at this point in the history
ArrayProxy was not properly updating its own chain tags whenever its
content updated, which caused the regression.

Fixing this caused another issue, since we were updating tags with more
recent state and not properly dirtying those tags. Since those tags were
consumed eagerly, earlier on in the render cycle, there was no way to
dirty the tags when the update occured lazily. So, we had to add a
`CUSTOM_TAG_FOR` to trigger the update _when_ the tag was consumed.
  • Loading branch information
Chris Garrett committed Feb 27, 2020
1 parent 7598547 commit 7b3840d
Show file tree
Hide file tree
Showing 3 changed files with 40 additions and 22 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, computed } from '@ember/-internals/metal';
import { get, set, notifyPropertyChange, computed, on } 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 @@ -1108,6 +1108,22 @@ moduleFor(
}
);

moduleFor(
'Syntax test: {{#each}} with array proxies, content is updated after init',
class extends EachTest {
createList(items) {
let wrapped = emberA(items);
let proxy = ArrayProxy.extend({
setup: on('init', function() {
this.set('content', emberA(wrapped));
}),
}).create();

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

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

let meta = peekMeta(array);

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

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

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

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

let cache = peekCacheFor(array);
Expand Down
31 changes: 18 additions & 13 deletions packages/@ember/-internals/runtime/lib/system/array_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,12 @@ import {
replace,
getChainTagsForKey,
tagForProperty,
arrayContentDidChange,
arrayContentWillChange,
CUSTOM_TAG_FOR,
} from '@ember/-internals/metal';
import EmberObject from './object';
import { isArray, MutableArray } from '../mixins/array';
import { assert } from '@ember/debug';
import { combine, update, validate, value } from '@glimmer/validator';
import { combine, update, validate, value, tagFor } from '@glimmer/validator';

const ARRAY_OBSERVER_MAPPING = {
willChange: '_arrangedContentArrayWillChange',
Expand Down Expand Up @@ -124,6 +123,16 @@ export default class ArrayProxy extends EmberObject {
this._removeArrangedContentArrayObserver();
}

[CUSTOM_TAG_FOR](key) {
if (key === '[]') {
// revalidate eagerly if we're being tracked, since we no longer will
// be able to later on due to backtracking re-render assertion
this._revalidate();
}

return tagFor(this, key);
}

/**
The content array. Must be an object that implements `Array` and/or
`MutableArray.`
Expand Down Expand Up @@ -311,6 +320,12 @@ export default class ArrayProxy extends EmberObject {

this._arrangedContentTag = combine(getChainTagsForKey(this, 'arrangedContent'));
this._arrangedContentRevision = value(this._arrangedContentTag);

let arrTag = tagForProperty(this, '[]');
let lengthTag = tagForProperty(this, 'length');

update(arrTag, combine(getChainTagsForKey(this, 'arrangedContent.[]')));
update(lengthTag, combine(getChainTagsForKey(this, 'arrangedContent.length')));
}
}
}
Expand All @@ -325,14 +340,4 @@ 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 7b3840d

Please sign in to comment.