Skip to content

Commit

Permalink
[BUGFIX][Tracked Properties] Prevents autotracking ArrayProxy creation
Browse files Browse the repository at this point in the history
This PR prevents an issue with inifinite rerendering that is
fundamentally caused by creating an array proxy and returning it from
a getter. This is commonly done with _promise arrays_, which proxy to
content that is fulfilled at a later point in time:

```js
export default CountrySelectComponent extends Component {
  @service store;

  get options() {
    return this.store.findAll('country');
  }
}
```

While it likely makes sense to memoize these types of requests, even in
that case, the cache will be busted by autotracking because the
_creation_ of the array proxy causes it's content to be tracked. When
the content is updated later, it will then invalidate the cache, causing
the value to be thrown away before it can even be used, and a new value
to be fetched. This loops infinitely.

To get around this, we realized that array proxies do not need to add
observers until they have been _interacted_ with in some way. We set the
`CONSUMED` flag on the proxy to indicate that this has happened, and
only add/remove observers if it has. Since this happens lazily, it is
_generally_ guaranteed to happen after the getter has returned, since it
usually doesn't make sense to both create a value and access it in the
same getter.
  • Loading branch information
Chris Garrett committed Apr 2, 2019
1 parent e2c828d commit b9ec819
Show file tree
Hide file tree
Showing 5 changed files with 74 additions and 15 deletions.
23 changes: 22 additions & 1 deletion packages/@ember/-internals/extension-support/lib/data_adapter.js
Original file line number Diff line number Diff line change
@@ -1,9 +1,30 @@
import { getOwner } from '@ember/-internals/owner';
import { scheduleOnce } from '@ember/runloop';
import { get, objectAt, addArrayObserver, removeArrayObserver } from '@ember/-internals/metal';
import {
get,
objectAt,
addArrayObserver as internalAddArrayObserver,
removeArrayObserver as internalRemoveArrayObserver,
} from '@ember/-internals/metal';
import { dasherize } from '@ember/string';
import { Namespace, Object as EmberObject, A as emberA } from '@ember/-internals/runtime';

function addArrayObserver(arr, target, observer) {
if (arr.addArrayObserver) {
arr.addArrayObserver(target, observer);
} else {
internalAddArrayObserver(arr, target, observer);
}
}

function removeArrayObserver(arr, target, observer) {
if (arr.removeArrayObserver) {
arr.removeArrayObserver(target, observer);
} else {
internalRemoveArrayObserver(arr, target, observer);
}
}

/**
@module @ember/debug
*/
Expand Down
51 changes: 43 additions & 8 deletions packages/@ember/-internals/runtime/lib/system/array_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@ import {
removeArrayObserver,
replace,
} from '@ember/-internals/metal';
import { _WeakSet as WeakSet } from '@ember/polyfills';
import EmberObject from './object';
import { isArray, MutableArray } from '../mixins/array';
import { assert } from '@ember/debug';

const CONSUMED = new WeakSet();

const ARRAY_OBSERVER_MAPPING = {
willChange: '_arrangedContentArrayWillChange',
didChange: '_arrangedContentArrayDidChange',
Expand Down Expand Up @@ -101,11 +104,12 @@ export default class ArrayProxy extends EmberObject {
this._length = 0;

this._arrangedContent = null;
this._addArrangedContentArrayObsever();
}

willDestroy() {
this._removeArrangedContentArrayObsever();
if (CONSUMED.has(this)) {
this._removeArrangedContentArrayObserver();
}
}

/**
Expand Down Expand Up @@ -164,6 +168,10 @@ export default class ArrayProxy extends EmberObject {

// Overriding objectAt is not supported.
objectAt(idx) {
if (!CONSUMED.has(this)) {
this._addArrangedContentArrayObserver();
}

if (this._objects === null) {
this._objects = [];
}
Expand All @@ -187,6 +195,10 @@ export default class ArrayProxy extends EmberObject {

// Overriding length is not supported.
get length() {
if (!CONSUMED.has(this)) {
this._addArrangedContentArrayObserver();
}

if (this._lengthDirty) {
let arrangedContent = get(this, 'arrangedContent');
this._length = arrangedContent ? get(arrangedContent, 'length') : 0;
Expand Down Expand Up @@ -217,24 +229,31 @@ export default class ArrayProxy extends EmberObject {
}

[PROPERTY_DID_CHANGE](key) {
if (!CONSUMED.has(this)) {
// The array proxy hasn't been accessed yet, nothing needs to be updated
return;
}

if (key === 'arrangedContent') {
let oldLength = this._objects === null ? 0 : this._objects.length;
let arrangedContent = get(this, 'arrangedContent');
let newLength = arrangedContent ? get(arrangedContent, 'length') : 0;

this._removeArrangedContentArrayObsever();
this._removeArrangedContentArrayObserver();
this.arrayContentWillChange(0, oldLength, newLength);

this._invalidate();

this.arrayContentDidChange(0, oldLength, newLength);
this._addArrangedContentArrayObsever();
this._addArrangedContentArrayObserver();
} else if (key === 'content') {
this._invalidate();
}
}

_addArrangedContentArrayObsever() {
_addArrangedContentArrayObserver() {
CONSUMED.add(this);

let arrangedContent = get(this, 'arrangedContent');
if (arrangedContent) {
assert("Can't set ArrayProxy's content to itself", arrangedContent !== this);
Expand All @@ -243,15 +262,23 @@ export default class ArrayProxy extends EmberObject {
isArray(arrangedContent) || arrangedContent.isDestroyed
);

addArrayObserver(arrangedContent, this, ARRAY_OBSERVER_MAPPING);
if (arrangedContent.addArrayObserver) {
arrangedContent.addArrayObserver(this, ARRAY_OBSERVER_MAPPING);
} else {
addArrayObserver(arrangedContent, this, ARRAY_OBSERVER_MAPPING);
}

this._arrangedContent = arrangedContent;
}
}

_removeArrangedContentArrayObsever() {
_removeArrangedContentArrayObserver() {
if (this._arrangedContent) {
removeArrayObserver(this._arrangedContent, this, ARRAY_OBSERVER_MAPPING);
if (this._arrangedContent.removeArrayObserver) {
this._arrangedContent.removeArrayObserver(this, ARRAY_OBSERVER_MAPPING);
} else {
removeArrayObserver(this._arrangedContent, this, ARRAY_OBSERVER_MAPPING);
}
}
}

Expand Down Expand Up @@ -291,4 +318,12 @@ ArrayProxy.reopen(MutableArray, {
@public
*/
arrangedContent: alias('content'),

addArrayObserver(target, opts) {
if (!CONSUMED.has(this)) {
this._addArrangedContentArrayObserver();
}

this._super(target, opts);
},
});
6 changes: 2 additions & 4 deletions packages/@ember/-internals/runtime/tests/helpers/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,6 @@ import {
get,
set,
computed,
addArrayObserver,
removeArrayObserver,
arrayContentWillChange,
arrayContentDidChange,
} from '@ember/-internals/metal';
Expand Down Expand Up @@ -63,12 +61,12 @@ const ArrayTestsObserverClass = EmberObject.extend({
},

observeArray(obj) {
addArrayObserver(obj, this);
obj.addArrayObserver(this);
return this;
},

stopObserveArray(obj) {
removeArrayObserver(obj, this);
obj.removeArrayObserver(this);
return this;
},

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import {
addObserver,
observer as emberObserver,
computed,
addArrayObserver,
removeArrayObserver,
arrayContentDidChange,
arrayContentWillChange,
Expand Down Expand Up @@ -195,7 +194,7 @@ moduleFor(
_after: null,
});

addArrayObserver(obj, observer);
obj.addArrayObserver(observer);
}

afterEach() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,9 @@ moduleFor(

proxy.set('content', content);

// consume the array
proxy.objectAt(0);

assert.deepEqual(sortedListenersFor(content, '@array:before'), [
'_arrangedContentArrayWillChange',
]);
Expand All @@ -39,6 +42,9 @@ moduleFor(
let content2 = A();
let proxy = ArrayProxy.create({ content: content1 });

// consume the array
proxy.objectAt(0);

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

0 comments on commit b9ec819

Please sign in to comment.