Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release] Make ArrayProxy Lazy #18770

Merged
merged 1 commit into from
Feb 28, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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