[BUGFIX] Tracked Props - Prevents autotracking ArrayProxy creation

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).

packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js

@@ -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';
@@ -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(),

packages/@ember/-internals/metal/lib/array.ts

@@ -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([]);
 
@@ -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);

packages/@ember/-internals/metal/lib/property_get.ts

@@ -8,7 +8,7 @@ import { DEBUG } from '@glimmer/env';
 import { descriptorForProperty } from './descriptor_map';
 import { isPath } from './path_cache';
 import { tagForProperty } from './tags';
-import { consume, isTracking } from './tracked';
+import { consume, deprecateMutationsInAutotrackingTransaction, isTracking } from './tracked';
 
 export const PROXY_CONTENT = symbol('PROXY_CONTENT');
 
@@ -143,7 +143,17 @@ export function get(obj: object, keyName: string): any {
       !(keyName in obj) &&
       typeof (obj as MaybeHasUnknownProperty).unknownProperty === 'function'
     ) {
-      return (obj as MaybeHasUnknownProperty).unknownProperty!(keyName);
+      if (DEBUG) {
+        let ret;
+
+        deprecateMutationsInAutotrackingTransaction!(() => {
+          ret = (obj as MaybeHasUnknownProperty).unknownProperty!(keyName);
+        });
+
+        return ret;
+      } else {
+        return (obj as MaybeHasUnknownProperty).unknownProperty!(keyName);
+      }
     }
   }
   return value;

packages/@ember/-internals/metal/lib/tracked.ts

@@ -13,7 +13,7 @@ interface AutotrackingTransactionSourceData {
   error: Error;
 }
 
-let WARN_IN_AUTOTRACKING_TRANSACTION = false;
+let DEPRECATE_IN_AUTOTRACKING_TRANSACTION = false;
 let AUTOTRACKING_TRANSACTION: WeakMap<Tag, AutotrackingTransactionSourceData> | null = null;
 
 export let runInAutotrackingTransaction: undefined | ((fn: () => void) => void);
@@ -28,40 +28,51 @@ export let assertTagNotConsumed:
 let markTagAsConsumed: undefined | ((_tag: Tag, sourceError: Error) => void);
 
 if (DEBUG) {
+  /**
+   * Creates a global autotracking transaction. This will prevent any backflow
+   * in any `track` calls within the transaction, even if they are not
+   * externally consumed.
+   *
+   * `runInAutotrackingTransaction` can be called within itself, and it will add
+   * onto the existing transaction if one exists.
+   *
+   * TODO: Only throw an error if the `track` is consumed.
+   */
   runInAutotrackingTransaction = (fn: () => void) => {
-    if (AUTOTRACKING_TRANSACTION !== null) {
-      // already in a transaction, continue and let the original transaction finish
-      fn();
-      return;
-    }
+    let previousDeprecateState = DEPRECATE_IN_AUTOTRACKING_TRANSACTION;
+    let previousTransactionState = AUTOTRACKING_TRANSACTION;
 
-    AUTOTRACKING_TRANSACTION = new WeakMap();
+    DEPRECATE_IN_AUTOTRACKING_TRANSACTION = false;
+
+    if (previousTransactionState === null) {
+      // if there was no transaction start it. Otherwise, the transaction already exists.
+      AUTOTRACKING_TRANSACTION = new WeakMap();
+    }
 
     try {
       fn();
     } finally {
-      AUTOTRACKING_TRANSACTION = null;
+      DEPRECATE_IN_AUTOTRACKING_TRANSACTION = previousDeprecateState;
+      AUTOTRACKING_TRANSACTION = previousTransactionState;
     }
   };
 
+  /**
+   * Switches to deprecating within an autotracking transaction, if one exists.
+   * If `runInAutotrackingTransaction` is called within the callback of this
+   * method, it switches back to throwing an error, allowing zebra-striping of
+   * the types of errors that are thrown.
+   *
+   * Does not start an autotracking transaction.
+   */
   deprecateMutationsInAutotrackingTransaction = (fn: () => void) => {
-    assert(
-      'deprecations can only occur inside of an autotracking transaction',
-      AUTOTRACKING_TRANSACTION !== null
-    );
-
-    if (WARN_IN_AUTOTRACKING_TRANSACTION) {
-      // already in a transaction, continue and let the original transaction finish
-      fn();
-      return;
-    }
-
-    WARN_IN_AUTOTRACKING_TRANSACTION = true;
+    let previousDeprecateState = DEPRECATE_IN_AUTOTRACKING_TRANSACTION;
+    DEPRECATE_IN_AUTOTRACKING_TRANSACTION = true;
 
     try {
       fn();
     } finally {
-      WARN_IN_AUTOTRACKING_TRANSACTION = false;
+      DEPRECATE_IN_AUTOTRACKING_TRANSACTION = previousDeprecateState;
     }
   };
 
@@ -137,7 +148,7 @@ if (DEBUG) {
 
     if (!sourceData) return;
 
-    if (WARN_IN_AUTOTRACKING_TRANSACTION && !forceHardError) {
+    if (DEPRECATE_IN_AUTOTRACKING_TRANSACTION && !forceHardError) {
       deprecate(makeAutotrackingErrorMessage(sourceData, obj, keyName), false, {
         id: 'autotracking.mutation-after-consumption',
         until: '4.0.0',
@@ -354,7 +365,7 @@ function descriptorForField([_target, key, desc]: [
     get(): any {
       let propertyTag = tagForProperty(this, key) as UpdatableTag;
 
-      if (CURRENT_TRACKER) CURRENT_TRACKER.add(propertyTag);
+      consume(propertyTag);
 
       let value;
 
@@ -378,6 +389,9 @@ function descriptorForField([_target, key, desc]: [
 
     set(newValue: any): void {
       if (DEBUG) {
+        // No matter what, attempting to update a tracked property in an
+        // autotracking context after it has been read is invalid, even if we
+        // are otherwise warning, so always assert.
         assertTagNotConsumed!(tagForProperty(this, key), this, key, true);
       }
 

packages/@ember/-internals/metal/tests/accessors/get_test.js

@@ -1,8 +1,9 @@
 import { ENV } from '@ember/-internals/environment';
 import { Object as EmberObject } from '@ember/-internals/runtime';
-import { get, getWithDefault, Mixin, observer, computed } from '../..';
+import { get, set, track, getWithDefault, Mixin, observer, computed } from '../..';
 import { moduleFor, AbstractTestCase } from 'internal-test-helpers';
 import { run } from '@ember/runloop';
+import { EMBER_METAL_TRACKED_PROPERTIES } from '@ember/canary-features';
 
 function aget(x, y) {
   return x[y];
@@ -290,6 +291,32 @@ moduleFor(
       }
     }
 
+    ['@test gives helpful deprecation when a property tracked with `get` is mutated after access within unknownProperty within an autotracking transaction'](
+      assert
+    ) {
+      if (!EMBER_METAL_TRACKED_PROPERTIES) {
+        assert.expect(0);
+        return;
+      }
+
+      class EmberObject {
+        foo = null;
+
+        unknownProperty() {
+          get(this, 'foo');
+          set(this, 'foo', 123);
+        }
+      }
+
+      let obj = new EmberObject();
+
+      expectDeprecation(() => {
+        track(() => {
+          get(obj, 'bar');
+        });
+      }, /You attempted to update `foo` on `EmberObject`, but it had already been used previously in the same computation/);
+    }
+
     // ..........................................................
     // BUGS
     //

packages/@ember/-internals/metal/tests/tracked/validation_test.js

@@ -348,14 +348,31 @@ if (EMBER_METAL_TRACKED_PROPERTIES) {
         let obj = new EmberObject();
 
         expectAssertion(() => {
-          if (DEBUG) {
-            track(() => {
-              obj.value;
-              obj.value = 123;
-            });
-          }
+          track(() => {
+            obj.value;
+            obj.value = 123;
+          });
         }, /You attempted to update `value` on `EmberObject`, but it had already been used previously in the same computation/);
       }
+
+      ['@test gives helpful assertion when a tracked property is mutated after access within unknownProperty within an autotracking transaction']() {
+        class EmberObject {
+          @tracked foo;
+
+          unknownProperty() {
+            this.foo;
+            this.foo = 123;
+          }
+        }
+
+        let obj = new EmberObject();
+
+        expectAssertion(() => {
+          track(() => {
+            get(obj, 'bar');
+          });
+        }, /You attempted to update `foo` on `EmberObject`, but it had already been used previously in the same computation/);
+      }
     }
   );
 }

packages/@ember/-internals/runtime/lib/mixins/array.js

@@ -19,6 +19,7 @@ import {
   removeArrayObserver,
   arrayContentWillChange,
   arrayContentDidChange,
+  nativeDescDecorator as descriptor,
 } from '@ember/-internals/metal';
 import { assert } from '@ember/debug';
 import Enumerable from './enumerable';
@@ -564,8 +565,12 @@ const ArrayMixin = Mixin.create(Enumerable, {
     @property {Boolean} hasArrayObservers
     @public
   */
-  hasArrayObservers: nonEnumerableComputed(function() {
-    return hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
+  hasArrayObservers: descriptor({
+    configurable: true,
+    enumerable: false,
+    get() {
+      hasListeners(this, '@array:change') || hasListeners(this, '@array:before');
+    },
   }),
 
   /**
@@ -695,7 +700,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);
@@ -725,7 +730,7 @@ const ArrayMixin = Mixin.create(Enumerable, {
 
   /**
     Alias for `mapBy`.
-    
+
     Returns the value of the named
     property on all items in the enumeration.
 

packages/@ember/-internals/runtime/lib/system/array_proxy.js

@@ -111,11 +111,11 @@ export default class ArrayProxy extends EmberObject {
       this._arrangedContentRevision = value(this._arrangedContentTag);
     }
 
-    this._addArrangedContentArrayObsever();
+    this._addArrangedContentArrayObserver();
   }
 
   willDestroy() {
-    this._removeArrangedContentArrayObsever();
+    this._removeArrangedContentArrayObserver();
   }
 
   /**
@@ -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);
@@ -275,7 +275,7 @@ export default class ArrayProxy extends EmberObject {
     }
   }
 
-  _removeArrangedContentArrayObsever() {
+  _removeArrangedContentArrayObserver() {
     if (this._arrangedContent) {
       removeArrayObserver(this._arrangedContent, this, ARRAY_OBSERVER_MAPPING);
     }