Skip to content

Commit

Permalink
[BUGFIX release] Ensures the arg proxy works with get
Browse files Browse the repository at this point in the history
Previously, the arg proxy used a custom system for passing along its
`capturedArgs` directly to `getChainTags`, in order to interoperate with
computed properties. As it turns out, there are a number of other places
where the correct tag needs to be gotten and setup.

This PR replaces the `UNKNOWN_PROPERTY_TAG` system with a more general
`CUSTOM_TAG_FOR` system. In this system, if we detect that an object has
this method, we defer to it to get the tag, even if the property was
defined on the object. There are two users of this system:

- ProxyMixin
- The arg proxy

Given that it's private, and we have relatively few use cases, I believe
this is the cleanest solution at the moment. The alternative would be to
keep `UNKNOWN_PROPERTY_TAG` and also add `CUSTOM_TAG_FOR`, but that
seems unnecessary given low usage.
  • Loading branch information
Chris Garrett committed Jan 29, 2020
1 parent f323419 commit fe4c13d
Show file tree
Hide file tree
Showing 7 changed files with 102 additions and 56 deletions.
27 changes: 17 additions & 10 deletions packages/@ember/-internals/glimmer/lib/component-managers/custom.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { ARGS_PROXY_TAGS } from '@ember/-internals/metal';
import { CUSTOM_TAG_FOR } from '@ember/-internals/metal';
import { Factory } from '@ember/-internals/owner';
import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
import { EMBER_CUSTOM_COMPONENT_ARG_PROXY } from '@ember/canary-features';
Expand Down Expand Up @@ -187,34 +187,41 @@ export default class CustomComponentManager<ComponentInstance>
): CustomComponentState<ComponentInstance> {
const { delegate } = definition;
const capturedArgs = args.capture();
const namedArgs = capturedArgs.named;

let value;
let namedArgsProxy = {};

if (EMBER_CUSTOM_COMPONENT_ARG_PROXY) {
let getTag = (key: string) => {
return namedArgs.get(key).tag;
};

if (HAS_NATIVE_PROXY) {
let handler: ProxyHandler<{}> = {
get(_target, prop) {
if (capturedArgs.named.has(prop as string)) {
let ref = capturedArgs.named.get(prop as string);
if (namedArgs.has(prop as string)) {
let ref = namedArgs.get(prop as string);
consume(ref.tag);

return ref.value();
} else if (prop === CUSTOM_TAG_FOR) {
return getTag;
}
},

has(_target, prop) {
return capturedArgs.named.has(prop as string);
return namedArgs.has(prop as string);
},

ownKeys(_target) {
return capturedArgs.named.names;
return namedArgs.names;
},

getOwnPropertyDescriptor(_target, prop) {
assert(
'args proxies do not have real property descriptors, so you should never need to call getOwnPropertyDescriptor yourself. This code exists for enumerability, such as in for-in loops and Object.keys()',
capturedArgs.named.has(prop as string)
namedArgs.has(prop as string)
);

return {
Expand All @@ -238,12 +245,14 @@ export default class CustomComponentManager<ComponentInstance>

namedArgsProxy = new Proxy(namedArgsProxy, handler);
} else {
capturedArgs.named.names.forEach(name => {
namedArgsProxy[CUSTOM_TAG_FOR] = getTag;

namedArgs.names.forEach(name => {
Object.defineProperty(namedArgsProxy, name, {
enumerable: true,
configurable: true,
get() {
let ref = capturedArgs.named.get(name);
let ref = namedArgs.get(name);
consume(ref.tag);

return ref.value();
Expand All @@ -252,8 +261,6 @@ export default class CustomComponentManager<ComponentInstance>
});
}

ARGS_PROXY_TAGS.set(namedArgsProxy, capturedArgs.named);

value = {
named: namedArgsProxy,
positional: capturedArgs.positional.value(),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { Object as EmberObject, A, ArrayProxy, PromiseProxyMixin } from '@ember/-internals/runtime';
import { EMBER_CUSTOM_COMPONENT_ARG_PROXY } from '@ember/canary-features';
import { computed, tracked, nativeDescDecorator as descriptor } from '@ember/-internals/metal';
import { computed, get, 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';
Expand Down Expand Up @@ -568,6 +568,50 @@ if (EMBER_CUSTOM_COMPONENT_ARG_PROXY) {
this.assertText('hello!');
}

'@test args can be accessed with get()'() {
class TestComponent extends GlimmerishComponent {
get text() {
return get(this, 'args.text');
}
}

this.registerComponent('test', {
ComponentClass: TestComponent,
template: '<p>{{this.text}}</p>',
});

this.render('<Test @text={{this.text}}/>', {
text: 'hello!',
});

this.assertText('hello!');

runTask(() => this.context.set('text', 'hello world!'));
this.assertText('hello world!');

runTask(() => this.context.set('text', 'hello!'));
this.assertText('hello!');
}

'@test args can be accessed with get() if no value is passed'() {
class TestComponent extends GlimmerishComponent {
get text() {
return get(this, 'args.text') || 'hello!';
}
}

this.registerComponent('test', {
ComponentClass: TestComponent,
template: '<p>{{this.text}}</p>',
});

this.render('<Test/>', {
text: 'hello!',
});

this.assertText('hello!');
}

'@test named args are enumerable'() {
class TestComponent extends GlimmerishComponent {
get objectKeys() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { RenderingTestCase, moduleFor, runTask } from 'internal-test-helpers';
import { set, get } from '@ember/-internals/metal';

import { Component } from '../../utils/helpers';
import GlimmerishComponent from '../../utils/glimmerish-component';

moduleFor(
'Helpers test: {{get}}',
Expand Down Expand Up @@ -616,5 +617,31 @@ moduleFor(

assert.strictEqual(this.$('#get-input').val(), 'mcintosh');
}

'@test should be able to get an object value with a path from this.args in a glimmer component'() {
class PersonComponent extends GlimmerishComponent {
options = ['first', 'last', 'age'];
}

this.registerComponent('person-wrapper', {
ComponentClass: PersonComponent,
template: '{{#each this.options as |option|}}{{get this.args option}}{{/each}}',
});

this.render('<PersonWrapper @first={{first}} @last={{last}} @age={{age}}/>', {
first: 'miguel',
last: 'andrade',
});

this.assertText('miguelandrade');

runTask(() => this.rerender());

this.assertText('miguelandrade');

runTask(() => set(this.context, 'age', 30));

this.assertText('miguelandrade30');
}
}
);
4 changes: 2 additions & 2 deletions packages/@ember/-internals/metal/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ export {
isClassicDecorator,
setClassicDecorator,
} from './lib/descriptor_map';
export { getChainTagsForKey, ARGS_PROXY_TAGS } from './lib/chain-tags';
export { getChainTagsForKey } from './lib/chain-tags';
export { default as libraries, Libraries } from './lib/libraries';
export { default as getProperties } from './lib/get_properties';
export { default as setProperties } from './lib/set_properties';
Expand All @@ -53,7 +53,7 @@ export { default as expandProperties } from './lib/expand_properties';
export { addObserver, activateObserver, removeObserver, flushAsyncObservers } from './lib/observer';
export { Mixin, aliasMethod, mixin, observer, applyMixin } from './lib/mixin';
export { default as inject, DEBUG_INJECTION_FUNCTIONS } from './lib/injected_property';
export { tagForProperty, tagForObject, markObjectAsDirty, UNKNOWN_PROPERTY_TAG } from './lib/tags';
export { tagForProperty, tagForObject, markObjectAsDirty, CUSTOM_TAG_FOR } from './lib/tags';
export { tracked } from './lib/tracked';

export {
Expand Down
36 changes: 0 additions & 36 deletions packages/@ember/-internals/metal/lib/chain-tags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,6 @@ import { getLastRevisionFor, peekCacheFor } from './computed_cache';
import { descriptorForProperty } from './descriptor_map';
import { tagForProperty } from './tags';

export const ARGS_PROXY_TAGS = new WeakMap();

export function finishLazyChains(obj: any, key: string, value: any) {
let meta = peekMeta(obj);
let lazyTags = meta !== null ? meta.readableLazyChainsFor(key) : undefined;
Expand Down Expand Up @@ -149,40 +147,6 @@ export function getChainTagsForKey(obj: any, path: string) {
break;
}

// If the segment is linking to an args proxy, we need to manually access
// the tags for the args, since they are direct references and don't have a
// tagForProperty. We then continue chaining like normal after it, since
// you could chain off an arg if it were an object, for instance.
if (segment === 'args' && ARGS_PROXY_TAGS.has(current.args)) {
assert(
`When watching the 'args' on a GlimmerComponent, you must watch a value on the args. You cannot watch the object itself, as it never changes.`,
segmentEnd !== pathLength
);

lastSegmentEnd = segmentEnd + 1;
segmentEnd = path.indexOf('.', lastSegmentEnd);

if (segmentEnd === -1) {
segmentEnd = pathLength;
}

segment = path.slice(lastSegmentEnd, segmentEnd)!;

let namedArgs = ARGS_PROXY_TAGS.get(current.args);
let ref = namedArgs.get(segment);

chainTags.push(ref.tag);

// We still need to break if we're at the end of the path.
if (segmentEnd === pathLength) {
break;
}

// Otherwise, set the current value and then continue to the next segment
current = ref.value();
continue;
}

// TODO: Assert that current[segment] isn't an undecorated, non-MANDATORY_SETTER/dependentKeyCompat getter

let propertyTag = tagForProperty(current, segment);
Expand Down
6 changes: 3 additions & 3 deletions packages/@ember/-internals/metal/lib/tags.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ if (DEBUG) {

/////////

export const UNKNOWN_PROPERTY_TAG = symbol('UNKNOWN_PROPERTY_TAG');
export const CUSTOM_TAG_FOR = symbol('CUSTOM_TAG_FOR');

// This is exported for `@tracked`, but should otherwise be avoided. Use `tagForObject`.
export const SELF_TAG: string = symbol('SELF_TAG');
Expand All @@ -66,8 +66,8 @@ export function tagForProperty(obj: unknown, propertyKey: string | symbol): Tag
return CONSTANT_TAG;
}

if (!(propertyKey in obj) && typeof obj[UNKNOWN_PROPERTY_TAG] === 'function') {
return obj[UNKNOWN_PROPERTY_TAG](propertyKey);
if (typeof obj[CUSTOM_TAG_FOR] === 'function') {
return obj[CUSTOM_TAG_FOR](propertyKey);
}

let tag = tagFor(obj, propertyKey);
Expand Down
12 changes: 8 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,12 @@ import {
Mixin,
tagForObject,
computed,
UNKNOWN_PROPERTY_TAG,
CUSTOM_TAG_FOR,
getChainTagsForKey,
} from '@ember/-internals/metal';
import { setProxy } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import { combine, update } from '@glimmer/validator';
import { combine, update, tagFor } from '@glimmer/validator';

export function contentFor(proxy) {
let content = get(proxy, 'content');
Expand Down Expand Up @@ -57,8 +57,12 @@ export default Mixin.create({
return Boolean(get(this, 'content'));
}),

[UNKNOWN_PROPERTY_TAG](key) {
return combine(getChainTagsForKey(this, `content.${key}`));
[CUSTOM_TAG_FOR](key) {
if (key in this) {
return tagFor(this, key);
} else {
return combine(getChainTagsForKey(this, `content.${key}`));
}
},

unknownProperty(key) {
Expand Down

0 comments on commit fe4c13d

Please sign in to comment.