From 57a204444d044e7e25e23b3a468d97bb33f4651c Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Mon, 28 Sep 2020 15:47:36 -0700 Subject: [PATCH] [BUGFIX lts] Fix tag cycles in query parameters Currently, we wrap query parameters with @dependentKeyCompat manually to ensure they work with the legacy router setup, which watches QPs using observers. When a QP value is labeled as @tracked, we don't need to replace it with @dependentKeyCompat, but since it's just a plain native getter (from the decorator) we do anyways. This results in a tag cycle being created, which can result in a negative performance impact, as every render pass will be invalidated by the cycle and require a subsequent full revalidation. This bug would have been caught by our ALLOW_CYCLES logic, but that logic was faulty - it was allowing too many cycles, anything that was accessed via get. It should only have allowed cycles from computeds themselves. This PR fixes the bug with ALLOW_CYCLES, and the subsequent bug with @tracked by only wrapping with @dependentKeyCompat if the value is not @tracked. It also adds an assertion to prevent users from using @dependentKeyCompat with @computed or @tracked. Verified that the test failed after fixing ALLOW_CYCLES, and passed again after the fix. --- packages/@ember/-internals/metal/lib/alias.ts | 3 +- .../@ember/-internals/metal/lib/chain-tags.ts | 13 ++---- .../@ember/-internals/metal/lib/computed.ts | 6 +++ .../@ember/-internals/metal/lib/tracked.ts | 46 ++++++++++++------- packages/@ember/object/compat.ts | 8 +++- .../ember/tests/routing/query_params_test.js | 40 ++++++++++++++++ 6 files changed, 90 insertions(+), 26 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/alias.ts b/packages/@ember/-internals/metal/lib/alias.ts index 6b1fe8ae701..e7b62f189b1 100644 --- a/packages/@ember/-internals/metal/lib/alias.ts +++ b/packages/@ember/-internals/metal/lib/alias.ts @@ -12,7 +12,7 @@ import { validateTag, valueForTag, } from '@glimmer/validator'; -import { finishLazyChains, getChainTagsForKey } from './chain-tags'; +import { CHAIN_PASS_THROUGH, finishLazyChains, getChainTagsForKey } from './chain-tags'; import { ComputedDescriptor, Decorator, @@ -70,6 +70,7 @@ export class AliasedProperty extends ComputedDescriptor { setup(obj: object, keyName: string, propertyDesc: PropertyDescriptor, meta: Meta): void { assert(`Setting alias '${keyName}' on self`, this.altKey !== keyName); super.setup(obj, keyName, propertyDesc, meta); + CHAIN_PASS_THROUGH.add(this); } get(obj: object, keyName: string): any { diff --git a/packages/@ember/-internals/metal/lib/chain-tags.ts b/packages/@ember/-internals/metal/lib/chain-tags.ts index 17c6c91c0e2..28e12577a44 100644 --- a/packages/@ember/-internals/metal/lib/chain-tags.ts +++ b/packages/@ember/-internals/metal/lib/chain-tags.ts @@ -1,9 +1,8 @@ import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta'; import { isObject } from '@ember/-internals/utils'; import { assert, deprecate } from '@ember/debug'; -import { DEBUG } from '@glimmer/env'; +import { _WeakSet } from '@glimmer/util'; import { - ALLOW_CYCLES, combine, createUpdatableTag, Tag, @@ -15,6 +14,8 @@ import { import { objectAt } from './array'; import { tagForProperty } from './tags'; +export const CHAIN_PASS_THROUGH = new _WeakSet(); + export function finishLazyChains(meta: Meta, key: string, value: any) { let lazyTags = meta.readableLazyChainsFor(key); @@ -166,7 +167,7 @@ function getChainTags( // If the key was an alias, we should always get the next value in order to // bootstrap the alias. This is because aliases, unlike other CPs, should // always be in sync with the aliased value. - if (descriptor !== undefined && typeof descriptor.altKey === 'string') { + if (CHAIN_PASS_THROUGH.has(descriptor)) { // tslint:disable-next-line: no-unused-expression current[segment]; } @@ -182,7 +183,7 @@ function getChainTags( } else { current = current[segment]; } - } else if (typeof descriptor.altKey === 'string') { + } else if (CHAIN_PASS_THROUGH.has(descriptor)) { current = current[segment]; } else { // If the descriptor is defined, then its a normal CP (not an alias, which @@ -218,9 +219,5 @@ function getChainTags( currentMeta = peekMeta(current); } - if (DEBUG) { - chainTags.forEach((t) => ALLOW_CYCLES!.set(t, true)); - } - return chainTags; } diff --git a/packages/@ember/-internals/metal/lib/computed.ts b/packages/@ember/-internals/metal/lib/computed.ts index 94088e597e7..f5850f23474 100644 --- a/packages/@ember/-internals/metal/lib/computed.ts +++ b/packages/@ember/-internals/metal/lib/computed.ts @@ -2,8 +2,10 @@ import { Meta, meta as metaFor } from '@ember/-internals/meta'; import { inspect, toString } from '@ember/-internals/utils'; import { assert, deprecate, warn } from '@ember/debug'; import EmberError from '@ember/error'; +import { DEBUG } from '@glimmer/env'; import { isDestroyed } from '@glimmer/runtime'; import { + ALLOW_CYCLES, consumeTag, tagFor, tagMetaFor, @@ -321,6 +323,10 @@ export class ComputedProperty extends ComputedDescriptor { ) ); + if (DEBUG) { + ALLOW_CYCLES!.set(tagFor(obj, keyName), true); + } + if (this._hasConfig === false) { assert( `Attempted to use @computed on ${keyName}, but it did not have a getter or a setter. You must either pass a get a function or getter/setter to @computed directly (e.g. \`@computed({ get() { ... } })\`) or apply @computed directly to a getter/setter`, diff --git a/packages/@ember/-internals/metal/lib/tracked.ts b/packages/@ember/-internals/metal/lib/tracked.ts index ef7eb1455aa..614297cbbeb 100644 --- a/packages/@ember/-internals/metal/lib/tracked.ts +++ b/packages/@ember/-internals/metal/lib/tracked.ts @@ -1,8 +1,11 @@ +import { meta as metaFor } from '@ember/-internals/meta'; import { isEmberArray } from '@ember/-internals/utils'; import { assert } from '@ember/debug'; import { DEBUG } from '@glimmer/env'; import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator'; +import { CHAIN_PASS_THROUGH } from './chain-tags'; import { + CP_SETTER_FUNCS, Decorator, DecoratorPropertyDescriptor, isElementDescriptor, @@ -142,7 +145,7 @@ if (DEBUG) { setClassicDecorator(tracked); } -function descriptorForField([_target, key, desc]: [ +function descriptorForField([target, key, desc]: [ object, string, DecoratorPropertyDescriptor @@ -154,25 +157,36 @@ function descriptorForField([_target, key, desc]: [ let { getter, setter } = trackedData(key, desc ? desc.initializer : undefined); - return { + function get(this: object): unknown { + let value = getter(this); + + // Add the tag of the returned value if it is an array, since arrays + // should always cause updates if they are consumed and then changed + if (Array.isArray(value) || isEmberArray(value)) { + consumeTag(tagFor(value, '[]')); + } + + return value; + } + + function set(this: object, newValue: unknown): void { + setter(this, newValue); + dirtyTagFor(this, SELF_TAG); + } + + let newDesc = { enumerable: true, configurable: true, + isTracked: true, - get(): any { - let value = getter(this); + get, + set, + }; - // Add the tag of the returned value if it is an array, since arrays - // should always cause updates if they are consumed and then changed - if (Array.isArray(value) || isEmberArray(value)) { - consumeTag(tagFor(value, '[]')); - } + metaFor(target).writeDescriptors(key, newDesc); - return value; - }, + CP_SETTER_FUNCS.add(set); + CHAIN_PASS_THROUGH.add(newDesc); - set(newValue: any): void { - setter(this, newValue); - dirtyTagFor(this, SELF_TAG); - }, - }; + return newDesc; } diff --git a/packages/@ember/object/compat.ts b/packages/@ember/object/compat.ts index 53bfafc5039..dc429f3135e 100644 --- a/packages/@ember/object/compat.ts +++ b/packages/@ember/object/compat.ts @@ -2,15 +2,21 @@ import { Meta } from '@ember/-internals/meta'; import { Decorator, DecoratorPropertyDescriptor, + descriptorForProperty, isElementDescriptor, setClassicDecorator, } from '@ember/-internals/metal'; import { assert } from '@ember/debug'; import { consumeTag, tagFor, track, UpdatableTag, updateTag } from '@glimmer/validator'; -let wrapGetterSetter = function (_target: object, key: string, desc: PropertyDescriptor) { +let wrapGetterSetter = function (target: object, key: string, desc: PropertyDescriptor) { let { get: originalGet } = desc; + assert( + 'You attempted to use @dependentKeyCompat on a property that already has been decorated with either @computed or @tracked. @dependentKeyCompat is only necessary for native getters that are not decorated with @computed.', + descriptorForProperty(target, key) === undefined + ); + if (originalGet !== undefined) { desc.get = function () { let propertyTag = tagFor(this, key) as UpdatableTag; diff --git a/packages/ember/tests/routing/query_params_test.js b/packages/ember/tests/routing/query_params_test.js index 15cf6f7a97f..b73a3ea4a72 100644 --- a/packages/ember/tests/routing/query_params_test.js +++ b/packages/ember/tests/routing/query_params_test.js @@ -1611,6 +1611,46 @@ moduleFor( this.assertCurrentPath('/?foo=987'); } + async ['@test Single query params defined with tracked properties can be linked to (and log is present)']( + assert + ) { + assert.expect(3); + + this.addTemplate( + 'application', + ` + + Home + + + 'Home (with params)' + + + + {{log this.foo}} + ` + ); + + this.add( + `controller:application`, + class extends Controller { + queryParams = ['foo', 'bar']; + @tracked foo = []; + @tracked bar = []; + } + ); + + await this.visitAndAssert('/'); + + document.getElementById('the-link').click(); + await runLoopSettled(); + this.assertCurrentPath('/'); + + document.getElementById('the-link-with-params').click(); + await runLoopSettled(); + this.assertCurrentPath('/?foo=%5B123%5D'); + } + async ['@test Single query params defined with native getters and tracked properties can be on the controller and reflected in the url']( assert ) {