Skip to content

Commit

Permalink
[BUGFIX beta] Ensure custom modifiers use args proxy.
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.

After this change, any modifiers using `modifierCapabilities('3.22')`
will only consume the arguments that are actually used (named and
positional). All modifiers that use `modifierCapabilities('3.13')` are
unchanged (preserving backwards compatibility).
  • Loading branch information
@rwjblue
rwjblue committed Sep 28, 2020
1 parent b49f91d commit d7f56c4
Show file tree
Hide file tree
Showing 3 changed files with 431 additions and 223 deletions.
112 changes: 69 additions & 43 deletions packages/@ember/-internals/glimmer/lib/modifiers/custom.ts
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
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 { Arguments, ModifierManager, VMArguments } from '@glimmer/interfaces';
import { registerDestructor, reifyArgs } from '@glimmer/runtime';
import { createUpdatableTag, untrack } from '@glimmer/validator';
import { createUpdatableTag, untrack, UpdatableTag } from '@glimmer/validator';
import { SimpleElement } from '@simple-dom/interface';
import { argsProxyFor } from '../utils/args-proxy';

export interface CustomModifierDefinitionState<ModifierInstance> {
ModifierClass: Factory<ModifierInstance>;
Expand All @@ -13,21 +14,33 @@ export interface CustomModifierDefinitionState<ModifierInstance> {
}

export interface OptionalCapabilities {
disableAutoTracking?: boolean;
'3.13': {
disableAutoTracking?: boolean;
};

// uses args proxy, does not provide a way to opt-out
'3.22': {
disableAutoTracking?: boolean;
};
}

export interface Capabilities {
disableAutoTracking: boolean;
useArgsProxy: boolean;
}

export function capabilities(
managerAPI: string,
optionalFeatures: OptionalCapabilities = {}
export function capabilities<Version extends keyof OptionalCapabilities>(
managerAPI: Version,
optionalFeatures: OptionalCapabilities[Version] = {}
): Capabilities {
assert('Invalid modifier manager compatibility specified', managerAPI === '3.13');
assert(
'Invalid modifier manager compatibility specified',
managerAPI === '3.13' || managerAPI === '3.22'
);

return {
disableAutoTracking: Boolean(optionalFeatures.disableAutoTracking),
useArgsProxy: managerAPI === '3.13' ? false : true,
};
}

Expand All @@ -53,32 +66,21 @@ export class CustomModifierDefinition<ModifierInstance> {
}
}

export class CustomModifierState<ModifierInstance> {
public tag = createUpdatableTag();
export interface CustomModifierState<ModifierInstance> {
tag: UpdatableTag;
element: SimpleElement;
delegate: ModifierManagerDelegate<ModifierInstance>;
modifier: ModifierInstance;
args: Arguments;
debugName?: string;

constructor(
public element: SimpleElement,
public delegate: ModifierManagerDelegate<ModifierInstance>,
public modifier: ModifierInstance,
public args: CapturedArguments
) {
registerDestructor(this, () => delegate.destroyModifier(modifier, reifyArgs(args)));
}
}

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

export interface ModifierManagerDelegate<ModifierInstance> {
capabilities: Capabilities;
createModifier(factory: unknown, args: Args): ModifierInstance;
installModifier(instance: ModifierInstance, element: SimpleElement, args: Args): void;
updateModifier(instance: ModifierInstance, args: Args): void;
destroyModifier(instance: ModifierInstance, args: Args): void;
createModifier(factory: unknown, args: Arguments): ModifierInstance;
installModifier(instance: ModifierInstance, element: SimpleElement, args: Arguments): void;
updateModifier(instance: ModifierInstance, args: Arguments): void;
destroyModifier(instance: ModifierInstance, args: Arguments): void;
}

/**
Expand Down Expand Up @@ -114,18 +116,49 @@ class InteractiveCustomModifierManager<ModifierInstance>
create(
element: SimpleElement,
definition: CustomModifierDefinitionState<ModifierInstance>,
args: VMArguments
vmArgs: VMArguments
) {
let { delegate, ModifierClass } = definition;
const capturedArgs = args.capture();
let capturedArgs = vmArgs.capture();

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

let instance = definition.delegate.createModifier(ModifierClass, reifyArgs(capturedArgs));
let state = new CustomModifierState(element, delegate, instance, capturedArgs);
let useArgsProxy = delegate.capabilities.useArgsProxy;

let args = useArgsProxy ? argsProxyFor(capturedArgs, 'modifier') : reifyArgs(capturedArgs);
let instance = delegate.createModifier(ModifierClass, args);

let tag = createUpdatableTag();
let state: CustomModifierState<ModifierInstance>;
if (useArgsProxy) {
state = {
tag,
element,
delegate,
args,
modifier: instance,
};
} else {
state = {
tag,
element,
delegate,
modifier: instance,
get args() {
return reifyArgs(capturedArgs);
},
};
}

if (DEBUG) {
state.debugName = definition.name;
}

registerDestructor(state, () => delegate.destroyModifier(instance, state.args));

return state;
}

Expand All @@ -140,30 +173,23 @@ class InteractiveCustomModifierManager<ModifierInstance>
install(state: CustomModifierState<ModifierInstance>) {
let { element, args, delegate, modifier } = 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);

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.installModifier(modifier, element, argsValue));
untrack(() => delegate.installModifier(modifier, element, args));
} else {
delegate.installModifier(modifier, element, argsValue);
delegate.installModifier(modifier, element, args);
}
}

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

if (capabilities.disableAutoTracking === true) {
untrack(() => delegate.updateModifier(modifier, argsValue));
untrack(() => delegate.updateModifier(modifier, args));
} else {
delegate.updateModifier(modifier, argsValue);
delegate.updateModifier(modifier, args);
}
}

Expand Down
26 changes: 18 additions & 8 deletions ...s/@ember/-internals/glimmer/tests/integration/components/angle-bracket-invocation-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1258,6 +1258,15 @@ moduleFor(
moduleFor(
'Element modifiers on AngleBracket components',
class extends RenderingTestCase {
assertNamedArgs(actual, expected, message) {
// `actual` is likely to be a named args proxy, while the `deepEqual` below would see
// the values as the same it would still flag as not deep equals because the constructors
// of the two objects do not match (one is a proxy, one is Object)
let reifiedActual = Object.assign({}, actual);

this.assert.deepEqual(reifiedActual, expected, message);
}

'@test modifiers are forwarded to a single element receiving the splattributes'(assert) {
let modifierParams = null;
let modifierNamedArgs = null;
Expand All @@ -1277,8 +1286,8 @@ moduleFor(
})
);
this.render('<TheFoo {{bar "something" foo="else"}}/>', {});
assert.deepEqual(modifierParams, ['something']);
assert.deepEqual(modifierNamedArgs, { foo: 'else' });
assert.deepEqual(modifierParams, ['something'], 'positional arguments');
this.assertNamedArgs(modifierNamedArgs, { foo: 'else' }, 'named arguments');
assert.equal(
modifiedElement && modifiedElement.getAttribute('id'),
'inner-div',
Expand All @@ -1293,12 +1302,13 @@ moduleFor(
template:
'<div id="inner-one" ...attributes>Foo</div><div id="inner-two" ...attributes>Bar</div>',
});
let test = this;
this.registerModifier(
'bar',
BaseModifier.extend({
didInsertElement(params, namedArgs) {
assert.deepEqual(params, ['something']);
assert.deepEqual(namedArgs, { foo: 'else' });
test.assertNamedArgs(namedArgs, { foo: 'else' });
if (this.element) {
elementIds.push(this.element.getAttribute('id'));
}
Expand Down Expand Up @@ -1341,15 +1351,15 @@ moduleFor(
foo: 'else',
});
assert.deepEqual(modifierParams, ['something']);
assert.deepEqual(modifierNamedArgs, { foo: 'else' });
this.assertNamedArgs(modifierNamedArgs, { foo: 'else' });
assert.equal(
modifiedElement && modifiedElement.getAttribute('id'),
'inner-div',
'Modifier is called on the element receiving the splattributes'
);
runTask(() => setProperties(this.context, { something: 'another', foo: 'thingy' }));
assert.deepEqual(modifierParams, ['another']);
assert.deepEqual(modifierNamedArgs, { foo: 'thingy' });
this.assertNamedArgs(modifierNamedArgs, { foo: 'thingy' });
assert.equal(
modifiedElement && modifiedElement.getAttribute('id'),
'inner-div',
Expand Down Expand Up @@ -1435,15 +1445,15 @@ moduleFor(
{ foo: 'bar' }
);
assert.deepEqual(modifierParams, ['bar']);
assert.deepEqual(modifierNamedArgs, { foo: 'bar' });
this.assertNamedArgs(modifierNamedArgs, { foo: 'bar' });
assert.equal(
modifiedElement && modifiedElement.getAttribute('id'),
'inner-div',
'Modifier is called on the element receiving the splattributes'
);
runTask(() => setProperties(this.context, { foo: 'qux' }));
assert.deepEqual(modifierParams, ['qux']);
assert.deepEqual(modifierNamedArgs, { foo: 'qux' });
this.assertNamedArgs(modifierNamedArgs, { foo: 'qux' });
assert.equal(
modifiedElement && modifiedElement.getAttribute('id'),
'inner-div',
Expand Down Expand Up @@ -1486,7 +1496,7 @@ moduleFor(
{ foo: 'bar' }
);
assert.deepEqual(modifierParams, ['bar']);
assert.deepEqual(modifierNamedArgs, { foo: 'bar' });
this.assertNamedArgs(modifierNamedArgs, { foo: 'bar' });
assert.deepEqual(
elementIds,
['outer-div', 'inner-div'],
Expand Down
Loading

0 comments on commit d7f56c4

Please sign in to comment.