Skip to content

Commit

Permalink
[BUGFIX release] Ensure closure actions are const
Browse files Browse the repository at this point in the history 
Fixes #14654
  • Loading branch information
@chancancode
chancancode committed Dec 6, 2016
1 parent 5a020a5 commit 585c54c
Show file tree
Hide file tree
Showing 2 changed files with 178 additions and 124 deletions.
190 changes: 97 additions & 93 deletions packages/ember-glimmer/lib/helpers/action.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,17 @@
@submodule ember-glimmer
*/
import { symbol } from 'ember-utils';
import { CachedReference } from '../utils/references';
import {
Error as EmberError,
run,
get,
flaggedInstrument,
isNone
isNone,
runInDebug
} from 'ember-metal';
import { UnboundReference } from '../utils/references';
import { EvaluatedPositionalArgs } from 'glimmer-runtime';
import { isConst } from 'glimmer-reference';

export const INVOKE = symbol('INVOKE');
export const ACTION = symbol('ACTION');
Expand Down Expand Up @@ -261,119 +264,120 @@ export const ACTION = symbol('ACTION');
@for Ember.Templates.helpers
@public
*/
export class ClosureActionReference extends CachedReference {
static create(args) {
// TODO: Const reference optimization.
return new ClosureActionReference(args);
}
export default function(vm, args) {
let { named, positional } = args;

constructor(args) {
super();
// The first two argument slots are reserved.
// pos[0] is the context (or `this`)
// pos[1] is the action name or function
// Anything else is an action argument.
let context = positional.at(0);
let action = positional.at(1);

this.args = args;
this.tag = args.tag;
}
// TODO: Is there a better way of doing this?
let debugKey = action._propertyKey;

compute() {
let { named, positional } = this.args;
let positionalValues = positional.value();

let target = positionalValues[0];
let rawActionRef = positional.at(1);
let rawAction = positionalValues[1];

// The first two argument slots are reserved.
// pos[0] is the context (or `this`)
// pos[1] is the action name or function
// Anything else is an action argument.
let actionArgs = positionalValues.slice(2);

// on-change={{action setName}}
// element-space actions look to "controller" then target. Here we only
// look to "target".
let actionType = typeof rawAction;
let action = rawAction;

if (rawActionRef[INVOKE]) {
target = rawActionRef;
action = rawActionRef[INVOKE];
} else if (isNone(rawAction)) {
throw new EmberError(`Action passed is null or undefined in (action) from ${target}.`);
} else if (actionType === 'string') {
// on-change={{action 'setName'}}
let actionName = rawAction;

action = null;

if (named.has('target')) {
// on-change={{action 'setName' target=alternativeComponent}}
target = named.get('target').value();
}
let restArgs;

if (target['actions']) {
action = target.actions[actionName];
}
if (positional.length === 2) {
restArgs = EvaluatedPositionalArgs.empty();
} else {
restArgs = EvaluatedPositionalArgs.create(positional.values.slice(2));
}

if (!action) {
throw new EmberError(`An action named '${actionName}' was not found in ${target}`);
}
} else if (action && typeof action[INVOKE] === 'function') {
target = action;
action = action[INVOKE];
} else if (actionType !== 'function') {
// TODO: Is there a better way of doing this?
let rawActionLabel = rawActionRef._propertyKey || rawAction;
throw new EmberError(`An action could not be made for \`${rawActionLabel}\` in ${target}. Please confirm that you are using either a quoted action name (i.e. \`(action '${rawActionLabel}')\`) or a function available in ${target}.`);
}
let target = named.has('target') ? named.get('target') : context;
let processArgs = makeArgsProcessor(named.has('value') && named.get('value'), restArgs);

let valuePath = named.get('value').value();
let fn;

return createClosureAction(target, action, valuePath, actionArgs);
if (typeof action[INVOKE] === 'function') {
fn = makeClosureAction(action, action, action[INVOKE], processArgs, debugKey);
} else if (isConst(target) && isConst(action)) {
fn = makeClosureAction(context.value(), target.value(), action.value(), processArgs, debugKey);
} else {
fn = makeDynamicClosureAction(context.value(), target, action, processArgs, debugKey);
}
}

export default function(vm, args) {
return ClosureActionReference.create(args);
fn[ACTION] = true;

return new UnboundReference(fn);
}

export function createClosureAction(target, action, valuePath, actionArgs) {
let closureAction;
let actionArgLength = actionArgs.length;
function NOOP(args) { return args; }

if (actionArgLength > 0) {
closureAction = function(...passedArguments) {
let args = new Array(actionArgLength + passedArguments.length);
function makeArgsProcessor(valuePathRef, actionArgsRef) {
let mergeArgs = null;

for (let i = 0; i < actionArgLength; i++) {
args[i] = actionArgs[i];
}
if (actionArgsRef.length > 0) {
mergeArgs = function(args) {
return actionArgsRef.value().concat(args);
};
}

for (let i = 0; i < passedArguments.length; i++) {
args[i + actionArgLength] = passedArguments[i];
}
let readValue = null;

if (valuePathRef) {
readValue = function(args) {
let valuePath = valuePathRef.value();

if (valuePath && args.length > 0) {
args[0] = get(args[0], valuePath);
}

let payload = { target, args, label: 'glimmer-closure-action' };
return flaggedInstrument('interaction.ember-action', payload, () => {
return run.join(target, action, ...args);
});
return args;
};
} else {
closureAction = function(...args) {
if (valuePath && args.length > 0) {
args[0] = get(args[0], valuePath);
}
}

let payload = { target, args, label: 'glimmer-closure-action' };
return flaggedInstrument('interaction.ember-action', payload, () => {
return run.join(target, action, ...args);
});
if (mergeArgs && readValue) {
return function(args) {
return readValue(mergeArgs(args));
};
} else {
return mergeArgs || readValue || NOOP;
}
}

function makeDynamicClosureAction(context, targetRef, actionRef, processArgs, debugKey) {
// We don't allow undefined/null, so this creates a throw-away action eagerly to trigger the error
runInDebug(function() {
makeClosureAction(context, targetRef.value(), actionRef.value(), processArgs, debugKey);
});

return function(...args) {
return makeClosureAction(context, targetRef.value(), actionRef.value(), processArgs, debugKey)(...args);
};
}

function makeClosureAction(context, target, action, processArgs, debugKey) {
let self, fn;

if (isNone(action)) {
throw new EmberError(`Action passed is null or undefined in (action) from ${target}.`);
} else if (typeof action[INVOKE] === 'function') {
self = action;
fn = action[INVOKE];
} else {
let typeofAction = typeof action;

if (typeofAction === 'string') {
self = target;
fn = target.actions && target.actions[action];

if (!fn) {
throw new EmberError(`An action named '${action}' was not found in ${target}`);
}
} else if (typeofAction === 'function') {
self = context;
fn = action;
} else {
throw new EmberError(`An action could not be made for \`${debugKey || action}\` in ${target}. Please confirm that you are using either a quoted action name (i.e. \`(action '${debugKey || 'myAction'}')\`) or a function available in ${target}.`);
}
}

closureAction[ACTION] = true;
return closureAction;
return function(...args) {
let payload = { target: self, args, label: 'glimmer-closure-action' };
return flaggedInstrument('interaction.ember-action', payload, () => {
return run.join(self, fn, ...processArgs(args));
});
};
}
112 changes: 81 additions & 31 deletions packages/ember-glimmer/tests/integration/helpers/closure-action-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
instrumentationUnsubscribe
} from 'ember-metal';
import { RenderingTest, moduleFor } from '../../utils/test-case';
import { strip } from '../../utils/abstract-test-case';
import { Component, INVOKE } from '../../utils/helpers';

if (isFeatureEnabled('ember-improved-instrumentation')) {
Expand Down Expand Up @@ -1104,52 +1105,101 @@ moduleFor('Helpers test: closure {{action}}', class extends RenderingTest {
this.assertText('Click me');
}

['@test ensure closure action transform does not cause incidental rerendering [GH#14305]'](assert) {
let counter = 0;
this.registerComponent('inner-component', {
template: 'Max',
['@test closure actions does not cause component hooks to fire unnecessarily [GH#14305] [GH#14654]'](assert) {
let clicked = 0;
let didReceiveAttrsFired = 0;

ComponentClass: Component.extend({
didReceiveAttrs() {
counter++;
}
})
let ClickMeComponent = Component.extend({
tagName: 'button',

click() {
this.get('onClick').call(undefined, ++clicked);
},

didReceiveAttrs() {
didReceiveAttrsFired++;
}
});

this.registerComponent('outer-component', {
template: '{{foo}} {{inner-component submit=(action "bar")}}',
this.registerComponent('click-me', {
ComponentClass: ClickMeComponent
});

ComponentClass: Component.extend({
actions: {
bar() { }
let outer;

let OuterComponent = Component.extend({
clicked: 0,

actions: {
'on-click': function() {
this.incrementProperty('clicked');
}
})
},

init() {
this._super();
outer = this;
this.set('onClick', () => this.incrementProperty('clicked'));
}
});

this.render('{{outer-component foo=foo derp=derp}}', {
foo: 'hi',
derp: 'nope!'
this.registerComponent('outer-component', {
ComponentClass: OuterComponent,
template: strip`
<div id="counter">clicked: {{clicked}}; foo: {{foo}}</div>
{{click-me id="string-action" onClick=(action "on-click")}}
{{click-me id="function-action" onClick=(action onClick)}}
{{click-me id="mut-action" onClick=(action (mut clicked))}}
`
});

this.assertText('hi Max');
assert.equal(counter, 1);
this.render('{{outer-component foo=foo}}', { foo: 1 });

this.assertStableRerender();
assert.equal(counter, 1);
this.assertText('clicked: 0; foo: 1');

assert.equal(didReceiveAttrsFired, 3);

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

this.assertText('clicked: 0; foo: 1');

assert.equal(didReceiveAttrsFired, 3);

this.runTask(() => set(this.context, 'foo', 2));

this.assertText('clicked: 0; foo: 2');

assert.equal(didReceiveAttrsFired, 3);

this.runTask(() => this.$('#string-action').click());

this.assertText('clicked: 1; foo: 2');

assert.equal(didReceiveAttrsFired, 3);

this.runTask(() => this.$('#function-action').click());

this.assertText('clicked: 2; foo: 2');

assert.equal(didReceiveAttrsFired, 3);

this.runTask(() => set(outer, 'onClick', function() { outer.incrementProperty('clicked'); }));

this.assertText('clicked: 2; foo: 2');

assert.equal(didReceiveAttrsFired, 3);

this.runTask(() => set(this.context, 'foo', 'bye'));
this.runTask(() => this.$('#function-action').click());

this.assertText('bye Max');
assert.equal(counter, 1);
this.assertText('clicked: 3; foo: 2');

this.runTask(() => set(this.context, 'foo', 'hi'));
assert.equal(didReceiveAttrsFired, 3);

this.assertText('hi Max');
assert.equal(counter, 1);
this.runTask(() => this.$('#mut-action').click());

this.runTask(() => set(this.context, 'derp', 'yup!'));
this.assertText('clicked: 4; foo: 2');

this.assertText('hi Max');
assert.equal(counter, 1);
assert.equal(didReceiveAttrsFired, 3);
}
});

0 comments on commit 585c54c

Please sign in to comment.