From da96a73a792c4b53ebbb8c961b2d547111c2fb4d Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Mon, 1 Apr 2019 14:07:38 -0700 Subject: [PATCH] [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). --- .../integration/components/tracked-test.js | 28 ++++++++- packages/@ember/-internals/metal/lib/array.ts | 3 +- .../-internals/metal/lib/property_get.ts | 14 ++++- .../@ember/-internals/metal/lib/tracked.ts | 60 ++++++++++++------- .../metal/tests/accessors/get_test.js | 29 ++++++++- .../metal/tests/tracked/validation_test.js | 29 +++++++-- .../-internals/runtime/lib/mixins/array.js | 13 ++-- .../runtime/lib/system/array_proxy.js | 12 ++-- 8 files changed, 143 insertions(+), 45 deletions(-) diff --git a/packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js b/packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js index 0fd0d5b1774..dc8ecf77309 100644 --- a/packages/@ember/-internals/glimmer/tests/integration/components/tracked-test.js +++ b/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(''); + + this.assertText('123'); + } + '@test tracked properties that are uninitialized do not throw an error'() { let CountComponent = Component.extend({ count: tracked(), diff --git a/packages/@ember/-internals/metal/lib/array.ts b/packages/@ember/-internals/metal/lib/array.ts index 45dd523ad0b..f3bf0f44847 100644 --- a/packages/@ember/-internals/metal/lib/array.ts +++ b/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); diff --git a/packages/@ember/-internals/metal/lib/property_get.ts b/packages/@ember/-internals/metal/lib/property_get.ts index 0251d4f4670..17f76f3b6c7 100644 --- a/packages/@ember/-internals/metal/lib/property_get.ts +++ b/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; diff --git a/packages/@ember/-internals/metal/lib/tracked.ts b/packages/@ember/-internals/metal/lib/tracked.ts index e51e8e9e0e9..4d4324ead38 100644 --- a/packages/@ember/-internals/metal/lib/tracked.ts +++ b/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 | 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); } diff --git a/packages/@ember/-internals/metal/tests/accessors/get_test.js b/packages/@ember/-internals/metal/tests/accessors/get_test.js index b3ce67788b3..6094431c971 100644 --- a/packages/@ember/-internals/metal/tests/accessors/get_test.js +++ b/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 // diff --git a/packages/@ember/-internals/metal/tests/tracked/validation_test.js b/packages/@ember/-internals/metal/tests/tracked/validation_test.js index 25530230d69..ffb09285d8e 100644 --- a/packages/@ember/-internals/metal/tests/tracked/validation_test.js +++ b/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/); + } } ); } diff --git a/packages/@ember/-internals/runtime/lib/mixins/array.js b/packages/@ember/-internals/runtime/lib/mixins/array.js index 7fe418c8e4d..aa84941c912 100644 --- a/packages/@ember/-internals/runtime/lib/mixins/array.js +++ b/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. diff --git a/packages/@ember/-internals/runtime/lib/system/array_proxy.js b/packages/@ember/-internals/runtime/lib/system/array_proxy.js index 2c52936ec41..f0917000689 100644 --- a/packages/@ember/-internals/runtime/lib/system/array_proxy.js +++ b/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); }