From 41b68b7ccb0720e08ab7011be8809bb929b7664c Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Tue, 13 Oct 2020 14:43:16 -0700 Subject: [PATCH] [BUGFIX] Asserts when users attempt to set a shadowed value with Ember.set --- .../@ember/-internals/metal/lib/decorator.ts | 39 ++++++++++++++----- .../-internals/metal/lib/property_set.ts | 24 ++++++++++-- .../@ember/-internals/metal/lib/tracked.ts | 7 ++++ .../metal/tests/tracked/set_test.js | 16 ++++++++ .../-internals/routing/lib/system/router.ts | 2 +- 5 files changed, 73 insertions(+), 15 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/decorator.ts b/packages/@ember/-internals/metal/lib/decorator.ts index a493e52bca5..bd172ec2271 100644 --- a/packages/@ember/-internals/metal/lib/decorator.ts +++ b/packages/@ember/-internals/metal/lib/decorator.ts @@ -1,5 +1,7 @@ import { Meta, meta as metaFor, peekMeta } from '@ember/-internals/meta'; import { assert } from '@ember/debug'; +import { DEBUG } from '@glimmer/env'; +import { _WeakSet as WeakSet } from '@glimmer/util'; export type DecoratorPropertyDescriptor = (PropertyDescriptor & { initializer?: any }) | undefined; @@ -69,19 +71,39 @@ export abstract class ComputedDescriptor { abstract set(obj: object, keyName: string, value: any | null | undefined): any | null | undefined; } -function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor): () => any { - return function CPGETTER_FUNCTION(this: object): any { +export let CPGETTERS: WeakSet<() => unknown>; +export let CPSETTERS: WeakSet<(value: unknown) => void>; + +if (DEBUG) { + CPGETTERS = new WeakSet(); + CPSETTERS = new WeakSet(); +} + +function DESCRIPTOR_GETTER_FUNCTION(name: string, descriptor: ComputedDescriptor): () => unknown { + function getter(this: object): unknown { return descriptor.get(this, name); - }; + } + + if (DEBUG) { + CPGETTERS.add(getter); + } + + return getter; } function DESCRIPTOR_SETTER_FUNCTION( name: string, descriptor: ComputedDescriptor -): (value: any) => void { - return function CPSETTER_FUNCTION(this: object, value: any): void { +): (value: unknown) => void { + function setter(this: object, value: unknown): void { return descriptor.set(this, name, value); - }; + } + + if (DEBUG) { + CPSETTERS.add(setter); + } + + return setter; } export function makeComputedDecorator( @@ -97,10 +119,7 @@ export function makeComputedDecorator( ): DecoratorPropertyDescriptor { assert( `Only one computed property decorator can be applied to a class field or accessor, but '${key}' was decorated twice. You may have added the decorator to both a getter and setter, which is unnecessary.`, - isClassicDecorator || - !propertyDesc || - !propertyDesc.get || - propertyDesc.get.toString().indexOf('CPGETTER_FUNCTION') === -1 + isClassicDecorator || !propertyDesc || !propertyDesc.get || !CPGETTERS.has(propertyDesc.get) ); let meta = arguments.length === 3 ? metaFor(target) : maybeMeta; diff --git a/packages/@ember/-internals/metal/lib/property_set.ts b/packages/@ember/-internals/metal/lib/property_set.ts index aef085266ed..3de6304a8bb 100644 --- a/packages/@ember/-internals/metal/lib/property_set.ts +++ b/packages/@ember/-internals/metal/lib/property_set.ts @@ -1,8 +1,13 @@ -import { HAS_NATIVE_PROXY, setWithMandatorySetter, toString } from '@ember/-internals/utils'; +import { + HAS_NATIVE_PROXY, + lookupDescriptor, + setWithMandatorySetter, + toString, +} from '@ember/-internals/utils'; import { assert } from '@ember/debug'; import EmberError from '@ember/error'; import { DEBUG } from '@glimmer/env'; -import { descriptorForProperty } from './decorator'; +import { CPSETTERS, descriptorForProperty } from './decorator'; import { isPath } from './path_cache'; import { notifyPropertyChange } from './property_events'; import { _getPath as getPath, getPossibleMandatoryProxyValue } from './property_get'; @@ -37,7 +42,7 @@ interface ExtendedObject { @return {Object} the passed value. @public */ -export function set(obj: object, keyName: string, value: any, tolerant?: boolean): any { +export function set(obj: object, keyName: string, value: T, tolerant?: boolean): T { assert( `Set must be called with three or four arguments; an object, a property key, a value and tolerant true/false`, arguments.length === 3 || arguments.length === 4 @@ -60,7 +65,7 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean `calling set on destroyed object: ${toString(obj)}.${keyName} = ${toString(value)}`, tolerant ); - return; + return value; } if (isPath(keyName)) { @@ -70,6 +75,17 @@ export function set(obj: object, keyName: string, value: any, tolerant?: boolean let descriptor = descriptorForProperty(obj, keyName); if (descriptor !== undefined) { + if (DEBUG) { + let instanceDesc = lookupDescriptor(obj, keyName); + + assert( + `Attempted to set \`${toString( + obj + )}.${keyName}\` using Ember.set(), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass.`, + instanceDesc && instanceDesc.set && CPSETTERS.has(instanceDesc.set) + ); + } + descriptor.set(obj, keyName, value); return value; } diff --git a/packages/@ember/-internals/metal/lib/tracked.ts b/packages/@ember/-internals/metal/lib/tracked.ts index e7399ead097..9469a6110d7 100644 --- a/packages/@ember/-internals/metal/lib/tracked.ts +++ b/packages/@ember/-internals/metal/lib/tracked.ts @@ -5,6 +5,8 @@ import { DEBUG } from '@glimmer/env'; import { consumeTag, dirtyTagFor, tagFor, trackedData } from '@glimmer/validator'; import { CHAIN_PASS_THROUGH } from './chain-tags'; import { + CPGETTERS, + CPSETTERS, Decorator, DecoratorPropertyDescriptor, isElementDescriptor, @@ -182,6 +184,11 @@ function descriptorForField([target, key, desc]: [ set, }; + if (DEBUG) { + CPGETTERS.add(get); + CPSETTERS.add(set); + } + metaFor(target).writeDescriptors(key, new TrackedDescriptor(get, set)); return newDesc; diff --git a/packages/@ember/-internals/metal/tests/tracked/set_test.js b/packages/@ember/-internals/metal/tests/tracked/set_test.js index 5b9eaa9a84d..6bed7598dea 100644 --- a/packages/@ember/-internals/metal/tests/tracked/set_test.js +++ b/packages/@ember/-internals/metal/tests/tracked/set_test.js @@ -31,5 +31,21 @@ moduleFor( assert.equal(get(newObj, key), obj[key], 'should set value'); } } + + ['@test set should throw an error when setting on shadowed properties']() { + class Obj { + @tracked value = 'emberjs'; + + constructor() { + Object.defineProperty(this, 'value', { writable: true, value: 'emberjs' }); + } + } + + let newObj = new Obj(); + + expectAssertion(() => { + set(newObj, 'value', 123); + }, /Attempted to set `\[object Object\].value` using Ember.set\(\), but the property was a computed or tracked property that was shadowed by another property declaration. This can happen if you defined a tracked or computed property on a parent class, and then redefined it on a subclass/); + } } ); diff --git a/packages/@ember/-internals/routing/lib/system/router.ts b/packages/@ember/-internals/routing/lib/system/router.ts index 8e08c6ca085..5f5796a1f58 100644 --- a/packages/@ember/-internals/routing/lib/system/router.ts +++ b/packages/@ember/-internals/routing/lib/system/router.ts @@ -671,7 +671,7 @@ class EmberRouter extends EmberObject { let owner = getOwner(this); if ('string' === typeof location && owner) { - let resolvedLocation = owner.lookup(`location:${location}`); + let resolvedLocation = owner.lookup(`location:${location}`); if (resolvedLocation !== undefined) { location = set(this, 'location', resolvedLocation);