Skip to content

Commit

Permalink
[BUGFIX lts] Use args proxy for modifier managers.
Browse files Browse the repository at this point in the history
Prior to this change, all custom modifiers **always** consumed every
argument on install/update/destroy. This was because `reifyArgs`
specifically reads all of them, and the actual usage was not
consumption based.
  • Loading branch information
rwjblue committed Sep 28, 2020
1 parent 984e69c commit ec868d1
Show file tree
Hide file tree
Showing 5 changed files with 208 additions and 122 deletions.
109 changes: 6 additions & 103 deletions packages/@ember/-internals/glimmer/lib/component-managers/custom.ts
Original file line number Diff line number Diff line change
@@ -1,13 +1,10 @@
import { ENV } from '@ember/-internals/environment';
import { CUSTOM_TAG_FOR } from '@ember/-internals/metal';
import { Factory } from '@ember/-internals/owner';
import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import {
Arguments,
Bounds,
CapturedArguments,
CapturedNamedArguments,
ComponentCapabilities,
ComponentDefinition,
Destroyable,
Expand All @@ -16,14 +13,14 @@ import {
VMArguments,
WithStaticLayout,
} from '@glimmer/interfaces';
import { createConstRef, Reference, valueForRef } from '@glimmer/reference';
import { createConstRef, Reference } from '@glimmer/reference';
import { registerDestructor, reifyPositional } from '@glimmer/runtime';
import { unwrapTemplate } from '@glimmer/util';
import { Tag, track } from '@glimmer/validator';
import { EmberVMEnvironment } from '../environment';
import RuntimeResolver from '../resolver';
import { OwnedTemplate } from '../template';
import AbstractComponentManager from './abstract';
import { namedArgsProxyFor } from '../utils/args-proxy';

const CAPABILITIES = {
dynamicLayout: false,
Expand Down Expand Up @@ -87,15 +84,9 @@ export interface Capabilities {
updateHook: boolean;
}

// TODO: export ICapturedArgumentsValue from glimmer and replace this
export interface Args {
named: Dict<unknown>;
positional: unknown[];
}

export interface ManagerDelegate<ComponentInstance> {
capabilities: Capabilities;
createComponent(factory: unknown, args: Args): ComponentInstance;
createComponent(factory: unknown, args: Arguments): ComponentInstance;
getContext(instance: ComponentInstance): unknown;
}

Expand All @@ -118,7 +109,7 @@ export function hasUpdateHook<ComponentInstance>(

export interface ManagerDelegateWithUpdateHook<ComponentInstance>
extends ManagerDelegate<ComponentInstance> {
updateComponent(instance: ComponentInstance, args: Args): void;
updateComponent(instance: ComponentInstance, args: Arguments): void;
}

export function hasAsyncUpdateHook<ComponentInstance>(
Expand Down Expand Up @@ -149,94 +140,6 @@ export interface ComponentArguments {
named: Dict<unknown>;
}

function tagForNamedArg<NamedArgs extends CapturedNamedArguments, K extends keyof NamedArgs>(
namedArgs: NamedArgs,
key: K
): Tag {
return track(() => valueForRef(namedArgs[key]));
}

let namedArgsProxyFor: (namedArgs: CapturedNamedArguments, debugName?: string) => Args['named'];

if (HAS_NATIVE_PROXY) {
namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(
namedArgs: NamedArgs,
debugName?: string
) => {
let getTag = (key: keyof Args) => tagForNamedArg(namedArgs, key);

let handler: ProxyHandler<{}> = {
get(_target, prop) {
let ref = namedArgs[prop as string];

if (ref !== undefined) {
return valueForRef(ref);
} else if (prop === CUSTOM_TAG_FOR) {
return getTag;
}
},

has(_target, prop) {
return namedArgs[prop as string] !== undefined;
},

ownKeys(_target) {
return Object.keys(namedArgs);
},

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()',
namedArgs[prop as string] !== undefined
);

return {
enumerable: true,
configurable: true,
};
},
};

if (DEBUG) {
handler.set = function(_target, prop) {
assert(
`You attempted to set ${debugName}#${String(
prop
)} on a components arguments. Component arguments are immutable and cannot be updated directly, they always represent the values that are passed to your component. If you want to set default values, you should use a getter instead`
);

return false;
};
}

return new Proxy({}, handler);
};
} else {
namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(namedArgs: NamedArgs) => {
let getTag = (key: keyof Args) => tagForNamedArg(namedArgs, key);

let proxy = {};

Object.defineProperty(proxy, CUSTOM_TAG_FOR, {
configurable: false,
enumerable: false,
value: getTag,
});

Object.keys(namedArgs).forEach(name => {
Object.defineProperty(proxy, name, {
enumerable: true,
configurable: true,
get() {
return valueForRef(namedArgs[name]);
},
});
});

return proxy;
};
}

/**
The CustomComponentManager allows addons to provide custom component
implementations that integrate seamlessly into Ember. This is accomplished
Expand Down Expand Up @@ -385,7 +288,7 @@ export class CustomComponentState<ComponentInstance> {
public component: ComponentInstance,
public args: CapturedArguments,
public env: EmberVMEnvironment,
public namedArgsProxy: Args['named']
public namedArgsProxy: Arguments['named']
) {
if (hasDestructors(delegate)) {
registerDestructor(this, () => delegate.destroyComponent(component));
Expand Down
46 changes: 35 additions & 11 deletions packages/@ember/-internals/glimmer/lib/modifiers/custom.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
import { Factory } from '@ember/-internals/owner';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { CapturedArguments, Dict, ModifierManager, VMArguments } from '@glimmer/interfaces';
import { registerDestructor, reifyArgs } from '@glimmer/runtime';
import {
Arguments,
CapturedArguments,
Dict,
ModifierManager,
VMArguments,
} from '@glimmer/interfaces';
import { registerDestructor, reifyPositional } from '@glimmer/runtime';
import { createUpdatableTag, untrack } from '@glimmer/validator';
import { SimpleElement } from '@simple-dom/interface';
import { namedArgsProxyFor } from '../utils/args-proxy';

export interface CustomModifierDefinitionState<ModifierInstance> {
ModifierClass: Factory<ModifierInstance>;
Expand Down Expand Up @@ -61,9 +68,15 @@ export class CustomModifierState<ModifierInstance> {
public element: SimpleElement,
public delegate: ModifierManagerDelegate<ModifierInstance>,
public modifier: ModifierInstance,
public args: CapturedArguments
public args: CapturedArguments,
public namedArgsProxy: Arguments['named']
) {
registerDestructor(this, () => delegate.destroyModifier(modifier, reifyArgs(args)));
registerDestructor(this, () =>
delegate.destroyModifier(modifier, {
positional: args.positional,
named: namedArgsProxy,
})
);
}
}

Expand Down Expand Up @@ -117,10 +130,15 @@ class InteractiveCustomModifierManager<ModifierInstance>
args: VMArguments
) {
let { delegate, ModifierClass } = definition;
const capturedArgs = args.capture();
let capturedArgs = args.capture();
let { named, positional } = capturedArgs;
let namedArgsProxy = namedArgsProxyFor(named);

let instance = definition.delegate.createModifier(ModifierClass, reifyArgs(capturedArgs));
let state = new CustomModifierState(element, delegate, instance, capturedArgs);
let instance = definition.delegate.createModifier(ModifierClass, {
positional: reifyPositional(positional),
named: namedArgsProxy,
});
let state = new CustomModifierState(element, delegate, instance, capturedArgs, namedArgsProxy);

if (DEBUG) {
state.debugName = definition.name;
Expand All @@ -138,15 +156,18 @@ class InteractiveCustomModifierManager<ModifierInstance>
}

install(state: CustomModifierState<ModifierInstance>) {
let { element, args, delegate, modifier } = state;
let { element, args, delegate, modifier, namedArgsProxy } = state;

assert(
'Custom modifier managers must define their capabilities using the capabilities() helper function',
typeof delegate.capabilities === 'object' && delegate.capabilities !== null
);

let { capabilities } = delegate;
let argsValue = reifyArgs(args);
let argsValue = {
positional: reifyPositional(args.positional),
named: namedArgsProxy,
};

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.installModifier(modifier, element, argsValue));
Expand All @@ -156,9 +177,12 @@ class InteractiveCustomModifierManager<ModifierInstance>
}

update(state: CustomModifierState<ModifierInstance>) {
let { args, delegate, modifier } = state;
let { args, delegate, modifier, namedArgsProxy } = state;
let { capabilities } = delegate;
let argsValue = reifyArgs(args);
let argsValue = {
positional: reifyPositional(args.positional),
named: namedArgsProxy,
};

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.updateModifier(modifier, argsValue));
Expand Down
98 changes: 98 additions & 0 deletions packages/@ember/-internals/glimmer/lib/utils/args-proxy.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
import { CUSTOM_TAG_FOR } from '@ember/-internals/metal';
import { HAS_NATIVE_PROXY } from '@ember/-internals/utils';
import { assert } from '@ember/debug';
import { DEBUG } from '@glimmer/env';
import { Arguments, CapturedNamedArguments } from '@glimmer/interfaces';
import { valueForRef } from '@glimmer/reference';
import { Tag, track } from '@glimmer/validator';

function tagForNamedArg<NamedArgs extends CapturedNamedArguments, K extends keyof NamedArgs>(
namedArgs: NamedArgs,
key: K
): Tag {
return track(() => valueForRef(namedArgs[key]));
}

export let namedArgsProxyFor: (
namedArgs: CapturedNamedArguments,
debugName?: string
) => Arguments['named'];

if (HAS_NATIVE_PROXY) {
namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(
namedArgs: NamedArgs,
debugName?: string
) => {
let getTag = (key: keyof Arguments) => tagForNamedArg(namedArgs, key);

let handler: ProxyHandler<{}> = {
get(_target, prop) {
let ref = namedArgs[prop as string];

if (ref !== undefined) {
return valueForRef(ref);
} else if (prop === CUSTOM_TAG_FOR) {
return getTag;
}
},

has(_target, prop) {
return namedArgs[prop as string] !== undefined;
},

ownKeys(_target) {
return Object.keys(namedArgs);
},

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()',
namedArgs[prop as string] !== undefined
);

return {
enumerable: true,
configurable: true,
};
},
};

if (DEBUG) {
handler.set = function(_target, prop) {
assert(
`You attempted to set ${debugName}#${String(
prop
)} on a components arguments. Component arguments are immutable and cannot be updated directly, they always represent the values that are passed to your component. If you want to set default values, you should use a getter instead`
);

return false;
};
}

return new Proxy({}, handler);
};
} else {
namedArgsProxyFor = <NamedArgs extends CapturedNamedArguments>(namedArgs: NamedArgs) => {
let getTag = (key: keyof Arguments) => tagForNamedArg(namedArgs, key);

let proxy = {};

Object.defineProperty(proxy, CUSTOM_TAG_FOR, {
configurable: false,
enumerable: false,
value: getTag,
});

Object.keys(namedArgs).forEach(name => {
Object.defineProperty(proxy, name, {
enumerable: true,
configurable: true,
get() {
return valueForRef(namedArgs[name]);
},
});
});

return proxy;
};
}
Loading

0 comments on commit ec868d1

Please sign in to comment.