From 27ad2effddef443c363b3aaba9a59d96eed4315e Mon Sep 17 00:00:00 2001 From: Chris Garrett Date: Tue, 28 Jan 2020 13:20:06 -0800 Subject: [PATCH] [BUGFIX release] Fixes tag chaining on Proxy mixins Currently, the Proxy mixin uses the private `UNKNOWN_PROPERTY_TAG` API to provide the tags for the content property that the mixin is proxying to. However, previously the chains system would do this _and_ still entangle changes to the local property. This allowed users to manually notify in subclasses of Proxy. This PR adds the tag for the property back to the returned tag, so it works the same as before. It also refactors `setupMandatorySetter`, since we now have to do that work in two places (to avoid re-entry). --- packages/@ember/-internals/metal/lib/tags.ts | 14 ++-------- .../-internals/runtime/lib/mixins/-proxy.js | 18 +++++++++--- .../runtime/tests/system/object_proxy_test.js | 28 +++++++++++++++++++ .../-internals/utils/lib/mandatory-setter.ts | 14 ++++++++-- 4 files changed, 57 insertions(+), 17 deletions(-) diff --git a/packages/@ember/-internals/metal/lib/tags.ts b/packages/@ember/-internals/metal/lib/tags.ts index 43bebc531de..a8ce2feb1a9 100644 --- a/packages/@ember/-internals/metal/lib/tags.ts +++ b/packages/@ember/-internals/metal/lib/tags.ts @@ -7,7 +7,6 @@ import { toString, } from '@ember/-internals/utils'; import { assert, deprecate } from '@ember/debug'; -import { _WeakSet as WeakSet } from '@ember/polyfills'; import { backburner } from '@ember/runloop'; import { DEBUG } from '@glimmer/env'; import { @@ -55,12 +54,6 @@ export const UNKNOWN_PROPERTY_TAG = symbol('UNKNOWN_PROPERTY_TAG'); // This is exported for `@tracked`, but should otherwise be avoided. Use `tagForObject`. export const SELF_TAG: string = symbol('SELF_TAG'); -let SEEN_TAGS: WeakSet | undefined; - -if (DEBUG) { - SEEN_TAGS = new WeakSet(); -} - export function tagForProperty(obj: unknown, propertyKey: string | symbol): Tag { if (!isObject(obj)) { return CONSTANT_TAG; @@ -72,11 +65,10 @@ export function tagForProperty(obj: unknown, propertyKey: string | symbol): Tag let tag = tagFor(obj, propertyKey); - if (DEBUG && !SEEN_TAGS!.has(tag)) { - SEEN_TAGS!.add(tag); - - setupMandatorySetter!(obj, propertyKey); + if (DEBUG) { + setupMandatorySetter!(tag, obj, propertyKey); + // TODO: Replace this with something more first class for tracking tags in DEBUG (tag as any)._propertyKey = propertyKey; } diff --git a/packages/@ember/-internals/runtime/lib/mixins/-proxy.js b/packages/@ember/-internals/runtime/lib/mixins/-proxy.js index 57c66f7c60b..8f61ea63343 100644 --- a/packages/@ember/-internals/runtime/lib/mixins/-proxy.js +++ b/packages/@ember/-internals/runtime/lib/mixins/-proxy.js @@ -10,12 +10,13 @@ import { Mixin, tagForObject, computed, - UNKNOWN_PROPERTY_TAG, getChainTagsForKey, + UNKNOWN_PROPERTY_TAG, } from '@ember/-internals/metal'; -import { setProxy } from '@ember/-internals/utils'; +import { setProxy, setupMandatorySetter } from '@ember/-internals/utils'; import { assert } from '@ember/debug'; -import { combine, update } from '@glimmer/validator'; +import { DEBUG } from '@glimmer/env'; +import { combine, update, tagFor } from '@glimmer/validator'; export function contentFor(proxy) { let content = get(proxy, 'content'); @@ -58,7 +59,16 @@ export default Mixin.create({ }), [UNKNOWN_PROPERTY_TAG](key) { - return combine(getChainTagsForKey(this, `content.${key}`)); + let tag = tagFor(this, key); + + if (DEBUG) { + setupMandatorySetter(tag, this, key); + + // TODO: Replace this with something more first class for tracking tags in DEBUG + tag._propertyKey = key; + } + + return combine([tag, ...getChainTagsForKey(this, `content.${key}`)]); }, unknownProperty(key) { diff --git a/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js b/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js index 9a2ecda3f62..c7c2d26179a 100644 --- a/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js +++ b/packages/@ember/-internals/runtime/tests/system/object_proxy_test.js @@ -338,5 +338,33 @@ moduleFor( observe: observer('foo', function() {}), }).create(); } + + async '@test custom proxies should be able to notify property changes manually'(assert) { + let proxy = ObjectProxy.extend({ + locals: { foo: 123 }, + + unknownProperty(key) { + return this.locals[key]; + }, + + setUnknownProperty(key, value) { + this.locals[key] = value; + this.notifyPropertyChange(key); + }, + }).create(); + + let count = 0; + + proxy.addObserver('foo', function() { + count++; + }); + + proxy.set('foo', 456); + await runLoopSettled(); + + assert.equal(count, 1); + assert.equal(proxy.get('foo'), 456); + assert.equal(proxy.get('locals.foo'), 456); + } } ); diff --git a/packages/@ember/-internals/utils/lib/mandatory-setter.ts b/packages/@ember/-internals/utils/lib/mandatory-setter.ts index c8dd855e49a..b7541877554 100644 --- a/packages/@ember/-internals/utils/lib/mandatory-setter.ts +++ b/packages/@ember/-internals/utils/lib/mandatory-setter.ts @@ -1,8 +1,10 @@ import { assert } from '@ember/debug'; +import { _WeakSet as WeakSet } from '@ember/polyfills'; import { DEBUG } from '@glimmer/env'; +import { Tag } from '@glimmer/validator'; import lookupDescriptor from './lookup-descriptor'; -export let setupMandatorySetter: ((obj: object, keyName: string | symbol) => void) | undefined; +export let setupMandatorySetter: ((tag: Tag, obj: object, keyName: string | symbol) => void) | undefined; export let teardownMandatorySetter: ((obj: object, keyName: string | symbol) => void) | undefined; export let setWithMandatorySetter: | ((obj: object, keyName: string | symbol, value: any) => void) @@ -11,6 +13,8 @@ export let setWithMandatorySetter: type PropertyDescriptorWithMeta = PropertyDescriptor & { hadOwnProperty?: boolean }; if (DEBUG) { + let SEEN_TAGS = new WeakSet(); + let MANDATORY_SETTERS: WeakMap< object, // @ts-ignore @@ -21,7 +25,13 @@ if (DEBUG) { return Object.prototype.propertyIsEnumerable.call(obj, key); }; - setupMandatorySetter = function(obj: object, keyName: string | symbol) { + setupMandatorySetter = function(tag: Tag, obj: object, keyName: string | symbol) { + if (SEEN_TAGS.has(tag)) { + return; + } + + SEEN_TAGS!.add(tag); + let desc = (lookupDescriptor(obj, keyName) as PropertyDescriptorWithMeta) || {}; if (desc.get || desc.set) {