Skip to content

Commit

Permalink
[BUGFIX release] Make ArrayProxy Lazy
Browse files Browse the repository at this point in the history
This PR refactors ArrayProxy to only begin observing the proxied array's
changes the first time the proxy is actually used. This fixes a number
of bugs that have popped up recently, and also ensures that ArrayProxy
creation does not accidentally entangle with the autotracking stack. It
also simplifies the code overall, and conceptually is more in line with
the way that autotracking works.

In addition, it adds a CUSTOM_TAG_FOR to the proxy, to forward the `[]`
and `length` tags from its content directly rather than attempting to
managed them itself. This keeps the system from encountering issues
related to tag updates, and from needing to re-notify when changes are
propagated.

As part of fixing this, I also discovered that the `hasArrayObservers`
API has been broken since ~3.11. I fixed it and added a test.

(cherry picked from commit 8bbedfa)
  • Loading branch information
Chris Garrett authored and kategengler committed Mar 3, 2020
1 parent 6ff73f9 commit 95430d1
Show file tree
Hide file tree
Showing 7 changed files with 97 additions and 30 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,31 @@ moduleFor(
this.assertText('123');
}

'@test creating an array proxy inside a tracking context and immediately updating its content before usage does not trigger backtracking assertion'() {
class LoaderComponent extends GlimmerishComponent {
get data() {
if (!this._data) {
this._data = ArrayProxy.create({
content: A(),
});

this._data.content.pushObjects([1, 2, 3]);
}

return this._data;
}
}

this.registerComponent('loader', {
ComponentClass: LoaderComponent,
template: '{{#each this.data as |item|}}{{item}}{{/each}}',
});

this.render('<Loader/>');

this.assertText('123');
}

'@test tracked properties that are uninitialized do not throw an error'() {
let CountComponent = Component.extend({
count: tracked(),
Expand Down
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
2 changes: 1 addition & 1 deletion packages/@ember/-internals/runtime/lib/mixins/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -569,7 +569,7 @@ const ArrayMixin = Mixin.create(Enumerable, {
configurable: true,
enumerable: false,
get() {
hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
return hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
},
}),

Expand Down
53 changes: 29 additions & 24 deletions packages/@ember/-internals/runtime/lib/system/array_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,13 @@ import {
removeArrayObserver,
replace,
getChainTagsForKey,
tagForProperty,
CUSTOM_TAG_FOR,
arrayContentDidChange,
arrayContentWillChange,
} 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, validate, value, tagFor } from '@glimmer/validator';

const ARRAY_OBSERVER_MAPPING = {
willChange: '_arrangedContentArrayWillChange',
Expand Down Expand Up @@ -106,18 +105,24 @@ export default class ArrayProxy extends EmberObject {
this._length = 0;

this._arrangedContent = null;

this._arrangedContentIsUpdating = false;
this._arrangedContentTag = combine(getChainTagsForKey(this, 'arrangedContent'));
this._arrangedContentRevision = value(this._arrangedContentTag);
this._arrangedContentTag = null;
this._arrangedContentRevision = null;
}

this._addArrangedContentArrayObserver();
[PROPERTY_DID_CHANGE]() {
this._revalidate();
}

update(tagForProperty(this, '[]'), combine(getChainTagsForKey(this, 'arrangedContent.[]')));
update(
tagForProperty(this, 'length'),
combine(getChainTagsForKey(this, 'arrangedContent.length'))
);
[CUSTOM_TAG_FOR](key) {
if (key === '[]' || key === 'length') {
// 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 combine(getChainTagsForKey(this, `arrangedContent.${key}`));
}

return tagFor(this, key);
}

willDestroy() {
Expand Down Expand Up @@ -236,10 +241,6 @@ export default class ArrayProxy extends EmberObject {
}
}

[PROPERTY_DID_CHANGE]() {
this._revalidate();
}

_updateArrangedContentArray() {
let oldLength = this._objects === null ? 0 : this._objects.length;
let arrangedContent = get(this, 'arrangedContent');
Expand Down Expand Up @@ -301,13 +302,21 @@ export default class ArrayProxy extends EmberObject {
}

_revalidate() {
if (this._arrangedContentIsUpdating === true) return;

if (
!this._arrangedContentIsUpdating &&
this._arrangedContentTag === null ||
!validate(this._arrangedContentTag, this._arrangedContentRevision)
) {
this._arrangedContentIsUpdating = true;
this._updateArrangedContentArray();
this._arrangedContentIsUpdating = false;
if (this._arrangedContentTag === null) {
// This is the first time the proxy has been setup, only add the observer
// don't trigger any events
this._addArrangedContentArrayObserver();
} else {
this._arrangedContentIsUpdating = true;
this._updateArrangedContentArray();
this._arrangedContentIsUpdating = false;
}

this._arrangedContentTag = combine(getChainTagsForKey(this, 'arrangedContent'));
this._arrangedContentRevision = value(this._arrangedContentTag);
Expand All @@ -328,10 +337,6 @@ ArrayProxy.reopen(MutableArray, {

// 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);
},
Expand Down
4 changes: 2 additions & 2 deletions packages/@ember/-internals/runtime/tests/helpers/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -314,7 +314,7 @@ export function runArrayTests(name, Tests, ...types) {
moduleFor(`EmberArray: ${name}`, Tests, EmberArrayHelpers);
break;
case 'MutableArray':
moduleFor(`MutableArray: ${name}`, Tests, EmberArrayHelpers);
moduleFor(`MutableArray: ${name}`, Tests, MutableArrayHelpers);
break;
case 'CopyableArray':
moduleFor(`CopyableArray: ${name}`, Tests, CopyableArray);
Expand All @@ -323,7 +323,7 @@ export function runArrayTests(name, Tests, ...types) {
moduleFor(`CopyableNativeArray: ${name}`, Tests, CopyableNativeArray);
break;
case 'NativeArray':
moduleFor(`NativeArray: ${name}`, Tests, EmberArrayHelpers);
moduleFor(`NativeArray: ${name}`, Tests, NativeArrayHelpers);
break;
}
});
Expand Down
19 changes: 17 additions & 2 deletions packages/@ember/-internals/runtime/tests/mixins/array_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import {
arrayContentWillChange,
} from '@ember/-internals/metal';
import EmberObject from '../../lib/system/object';
import EmberArray from '../../lib/mixins/array';
import { A as emberA } from '../../lib/mixins/array';
import EmberArray, { A as emberA } from '../../lib/mixins/array';
import { moduleFor, AbstractTestCase, runLoopSettled } from 'internal-test-helpers';

/*
Expand Down Expand Up @@ -254,6 +253,22 @@ moduleFor(
arrayContentDidChange(obj);
assert.deepEqual(observer._after, null);
}

['@test hasArrayObservers should work'](assert) {
assert.equal(
obj.hasArrayObservers,
true,
'correctly shows it has an array observer when one exists'
);

removeArrayObserver(obj, observer);

assert.equal(
obj.hasArrayObservers,
false,
'correctly shows it has an array observer when one exists'
);
}
}
);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,12 @@ moduleFor(
let content2 = A();
let proxy = ArrayProxy.create({ content: content1 });

assert.deepEqual(sortedListenersFor(content1, '@array:before'), []);
assert.deepEqual(sortedListenersFor(content1, '@array:change'), []);

// setup proxy
proxy.length;

assert.deepEqual(sortedListenersFor(content1, '@array:before'), [
'_arrangedContentArrayWillChange',
]);
Expand Down

0 comments on commit 95430d1

Please sign in to comment.