Skip to content

Commit

Permalink
[BUGFIX release] Fixes tag chaining on Proxy mixins
Browse files Browse the repository at this point in the history
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).
  • Loading branch information
Chris Garrett committed Jan 28, 2020
1 parent f323419 commit 27ad2ef
Show file tree
Hide file tree
Showing 4 changed files with 57 additions and 17 deletions.
14 changes: 3 additions & 11 deletions packages/@ember/-internals/metal/lib/tags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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<Tag> | undefined;

if (DEBUG) {
SEEN_TAGS = new WeakSet();
}

export function tagForProperty(obj: unknown, propertyKey: string | symbol): Tag {
if (!isObject(obj)) {
return CONSTANT_TAG;
Expand All @@ -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;
}

Expand Down
18 changes: 14 additions & 4 deletions packages/@ember/-internals/runtime/lib/mixins/-proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down Expand Up @@ -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) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
);
14 changes: 12 additions & 2 deletions packages/@ember/-internals/utils/lib/mandatory-setter.ts
Original file line number Diff line number Diff line change
@@ -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)
Expand All @@ -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
Expand All @@ -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) {
Expand Down

0 comments on commit 27ad2ef

Please sign in to comment.