Skip to content

Commit

Permalink
[BUGFIX] Tracked Props - Prevents autotracking ArrayProxy creation
Browse files Browse the repository at this point in the history
This PR prevents an issue with immediate invalidation within the new
autotracking transaction. ArrayProxy currently reads from one of its
own properties, `hasArrayObservers`, and then immediately updates it
with `notifyPropertyChange`.

We avoid this by using a native descriptor instead of a computed, and
not using `get()` to access it. This way, nothing is tracked during
creation (even if it is mutated).
  • Loading branch information
Chris Garrett committed Nov 21, 2019
1 parent d38232f commit 1f67007
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 13 deletions.
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
import { Object as EmberObject, A } from '@ember/-internals/runtime';
import { Object as EmberObject, A, ArrayProxy, PromiseProxyMixin } from '@ember/-internals/runtime';
import {
EMBER_CUSTOM_COMPONENT_ARG_PROXY,
EMBER_METAL_TRACKED_PROPERTIES,
} from '@ember/canary-features';
import { computed, tracked, nativeDescDecorator as descriptor } from '@ember/-internals/metal';
import { Promise } from 'rsvp';
import { moduleFor, RenderingTestCase, strip, runTask } from 'internal-test-helpers';
import GlimmerishComponent from '../../utils/glimmerish-component';
import { Component } from '../../utils/helpers';
Expand Down Expand Up @@ -47,6 +48,31 @@ if (EMBER_METAL_TRACKED_PROPERTIES) {
this.assertText('max jackson | max jackson');
}

'@test creating an array proxy inside a tracking context does not trigger backtracking assertion'() {
let PromiseArray = ArrayProxy.extend(PromiseProxyMixin);

class LoaderComponent extends GlimmerishComponent {
get data() {
if (!this._data) {
this._data = PromiseArray.create({
promise: Promise.resolve([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
3 changes: 1 addition & 2 deletions packages/@ember/-internals/metal/lib/array.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import { arrayContentDidChange, arrayContentWillChange } from './array_events';
import { addListener, removeListener } from './events';
import { notifyPropertyChange } from './property_events';
import { get } from './property_get';

const EMPTY_ARRAY = Object.freeze([]);

Expand Down Expand Up @@ -84,7 +83,7 @@ function arrayObserversHelper(
): ObjectHasArrayObservers {
let willChange = (opts && opts.willChange) || 'arrayWillChange';
let didChange = (opts && opts.didChange) || 'arrayDidChange';
let hasObservers = get(obj, 'hasArrayObservers');
let hasObservers = obj.hasArrayObservers;

operation(obj, '@array:before', target, willChange);
operation(obj, '@array:change', target, didChange);
Expand Down
11 changes: 7 additions & 4 deletions packages/@ember/-internals/runtime/lib/mixins/array.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
removeArrayObserver,
arrayContentWillChange,
arrayContentDidChange,
nativeDescDecorator as descriptor
} from '@ember/-internals/metal';
import { assert } from '@ember/debug';
import Enumerable from './enumerable';
Expand Down Expand Up @@ -564,8 +565,10 @@ const ArrayMixin = Mixin.create(Enumerable, {
@property {Boolean} hasArrayObservers
@public
*/
hasArrayObservers: nonEnumerableComputed(function() {
return hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
hasArrayObservers: descriptor({
get() {
hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
}
}),

/**
Expand Down Expand Up @@ -695,7 +698,7 @@ const ArrayMixin = Mixin.create(Enumerable, {
foods.forEach((food) => food.eaten = true);
let output = '';
foods.forEach((item, index, array) =>
foods.forEach((item, index, array) =>
output += `${index + 1}/${array.length} ${item.name}\n`;
);
console.log(output);
Expand Down Expand Up @@ -725,7 +728,7 @@ const ArrayMixin = Mixin.create(Enumerable, {

/**
Alias for `mapBy`.
Returns the value of the named
property on all items in the enumeration.
Expand Down
12 changes: 6 additions & 6 deletions packages/@ember/-internals/runtime/lib/system/array_proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -111,11 +111,11 @@ export default class ArrayProxy extends EmberObject {
this._arrangedContentRevision = value(this._arrangedContentTag);
}

this._addArrangedContentArrayObsever();
this._addArrangedContentArrayObserver();
}

willDestroy() {
this._removeArrangedContentArrayObsever();
this._removeArrangedContentArrayObserver();
}

/**
Expand Down Expand Up @@ -251,16 +251,16 @@ export default class ArrayProxy extends EmberObject {
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();
}

_addArrangedContentArrayObsever() {
_addArrangedContentArrayObserver() {
let arrangedContent = get(this, 'arrangedContent');
if (arrangedContent && !arrangedContent.isDestroyed) {
assert("Can't set ArrayProxy's content to itself", arrangedContent !== this);
Expand All @@ -275,7 +275,7 @@ export default class ArrayProxy extends EmberObject {
}
}

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

0 comments on commit 1f67007

Please sign in to comment.