Skip to content

Commit

Permalink
[BUGFIX release] Ensures the arg proxy works with get (#18708)
Browse files Browse the repository at this point in the history
[BUGFIX release] Ensures the arg proxy works with `get`
  • Loading branch information
rwjblue authored Jan 30, 2020
2 parents 99e796a + 6e8eb68 commit a3ee32e
Show file tree
Hide file tree
Showing 7 changed files with 106 additions and 56 deletions.
31 changes: 21 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,18 @@ export default class CustomComponentManager<ComponentInstance>

namedArgsProxy = new Proxy(namedArgsProxy, handler);
} else {
capturedArgs.named.names.forEach(name => {
Object.defineProperty(namedArgsProxy, CUSTOM_TAG_FOR, {
configurable: false,
enumerable: false,
value: 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 +265,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 a3ee32e

Please sign in to comment.