Skip to content

Commit

Permalink
[DEPRECATION] Deprecate Component#sendAction (RFC #335)
Browse files Browse the repository at this point in the history 
This also implicitly deprecated passing actions to the built-in inputs like `{{input enter="foo"}},
instead users must do `{{input enter=(action "foo")}}`.
There is a build-time deprecation that uses AST visitors to output precise information of file
and line the offending code is.
There is also a runtime-deprecation for cases that the AST deprecation can't catch.
  • Loading branch information
Miguel Camba committed Jun 18, 2018
1 parent 396424c commit b21f77b
Show file tree
Hide file tree
Showing 12 changed files with 411 additions and 140 deletions.
1 change: 1 addition & 0 deletions packages/@ember/deprecated-features/index.ts
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
export const SEND_ACTION = !!'3.4.0';
export const PROPERTY_BASED_DESCRIPTORS = !!'3.2.0';
export const EMBER_EXTEND_PROTOTYPES = !!'3.2.0-beta.5';
export const DEPRECATE_OPTIONS_MISSING = !!'2.1.0-beta.1';
Expand Down
8 changes: 6 additions & 2 deletions packages/ember-glimmer/tests/integration/components/dynamic-components-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -435,7 +435,11 @@ moduleFor(
classNames: 'inner-component',
didInsertElement() {
// trigger action on click in absence of app's EventDispatcher
let sendAction = (this.eventHandler = () => this.sendAction('somethingClicked'));
let sendAction = (this.eventHandler = () => {
if (this.somethingClicked) {
this.somethingClicked();
}
});
this.element.addEventListener('click', sendAction);
},
willDestroyElement() {
Expand All @@ -447,7 +451,7 @@ moduleFor(
let actionTriggered = 0;
this.registerComponent('outer-component', {
template:
'{{#component componentName somethingClicked="mappedAction"}}arepas!{{/component}}',
'{{#component componentName somethingClicked=(action "mappedAction")}}arepas!{{/component}}',
ComponentClass: Component.extend({
classNames: 'outer-component',
componentName: 'inner-component',
Expand Down
152 changes: 101 additions & 51 deletions packages/ember-glimmer/tests/integration/components/target-action-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ import Controller from '@ember/controller';
import { Object as EmberObject } from 'ember-runtime';
import { Route } from 'ember-routing';

function expectSendActionDeprecation(fn) {
expectDeprecation(
fn,
/You called (.*).sendAction\((.*)\) but Component#sendAction is deprecated. Please use closure actions instead./
);
}

moduleFor(
'Components test: sendAction',
class extends RenderingTest {
Expand Down Expand Up @@ -61,45 +68,52 @@ moduleFor(
['@test Calling sendAction on a component without an action defined does nothing']() {
this.renderDelegate();

this.runTask(() => this.delegate.sendAction());
expectSendActionDeprecation(() => {
this.runTask(() => this.delegate.sendAction());
});

this.assertSendCount(0);
}

['@test Calling sendAction on a component with an action defined calls send on the controller']() {
this.renderDelegate();

this.runTask(() => {
set(this.delegate, 'action', 'addItem');
this.delegate.sendAction();
expectSendActionDeprecation(() => {
this.runTask(() => {
set(this.delegate, 'action', 'addItem');
this.delegate.sendAction();
});
});

this.assertSendCount(1);
this.assertNamedSendCount('addItem', 1);
}

['@test Calling sendAction on a component with a function calls the function']() {
this.assert.expect(1);
this.assert.expect(2);

this.renderDelegate();

this.runTask(() => {
set(this.delegate, 'action', () => this.assert.ok(true, 'function is called'));
this.delegate.sendAction();
expectSendActionDeprecation(() => {
this.runTask(() => {
set(this.delegate, 'action', () => this.assert.ok(true, 'function is called'));
this.delegate.sendAction();
});
});
}

['@test Calling sendAction on a component with a function calls the function with arguments']() {
this.assert.expect(1);
this.assert.expect(2);
let argument = {};

this.renderDelegate();

this.runTask(() => {
set(this.delegate, 'action', actualArgument => {
this.assert.deepEqual(argument, actualArgument, 'argument is passed');
expectSendActionDeprecation(() => {
this.runTask(() => {
set(this.delegate, 'action', actualArgument => {
this.assert.deepEqual(argument, actualArgument, 'argument is passed');
});
this.delegate.sendAction('action', argument);
});
this.delegate.sendAction('action', argument);
});
}

Expand All @@ -109,16 +123,19 @@ moduleFor(
playing: null,
});

this.runTask(() => this.delegate.sendAction());
expectSendActionDeprecation(() => {
this.runTask(() => this.delegate.sendAction());
});

this.assertSendCount(0);

this.runTask(() => {
set(this.context, 'playing', 'didStartPlaying');
});

this.runTask(() => {
this.delegate.sendAction('playing');
expectSendActionDeprecation(() => {
this.runTask(() => {
this.delegate.sendAction('playing');
});
});

this.assertSendCount(1);
Expand All @@ -130,12 +147,16 @@ moduleFor(
playing: null,
});

this.runTask(() => this.delegate.sendAction('playing'));
expectSendActionDeprecation(() => {
this.runTask(() => this.delegate.sendAction('playing'));
});

this.assertSendCount(0);

this.runTask(() => this.delegate.attrs.playing.update('didStartPlaying'));
this.runTask(() => this.delegate.sendAction('playing'));
expectSendActionDeprecation(() => {
this.runTask(() => this.delegate.sendAction('playing'));
});

this.assertSendCount(1);
this.assertNamedSendCount('didStartPlaying', 1);
Expand All @@ -145,23 +166,28 @@ moduleFor(
this.renderDelegate();

let component = this.delegate;

this.runTask(() => {
set(this.delegate, 'playing', 'didStartPlaying');
component.sendAction('playing');
expectSendActionDeprecation(() => {
this.runTask(() => {
set(this.delegate, 'playing', 'didStartPlaying');
component.sendAction('playing');
});
});

this.assertSendCount(1);
this.assertNamedSendCount('didStartPlaying', 1);

this.runTask(() => component.sendAction('playing'));
expectSendActionDeprecation(() => {
this.runTask(() => component.sendAction('playing'));
});

this.assertSendCount(2);
this.assertNamedSendCount('didStartPlaying', 2);

this.runTask(() => {
set(component, 'action', 'didDoSomeBusiness');
component.sendAction();
expectSendActionDeprecation(() => {
this.runTask(() => {
set(component, 'action', 'didDoSomeBusiness');
component.sendAction();
});
});

this.assertSendCount(3);
Expand All @@ -175,9 +201,12 @@ moduleFor(
set(this.delegate, 'action', {});
set(this.delegate, 'playing', {});
});

expectAssertion(() => this.delegate.sendAction());
expectAssertion(() => this.delegate.sendAction('playing'));
expectSendActionDeprecation(() => {
expectAssertion(() => this.delegate.sendAction());
});
expectSendActionDeprecation(() => {
expectAssertion(() => this.delegate.sendAction('playing'));
});
}

['@test Calling sendAction on a component with contexts']() {
Expand All @@ -187,17 +216,21 @@ moduleFor(
let firstContext = { song: 'She Broke My Ember' };
let secondContext = { song: 'My Achey Breaky Ember' };

this.runTask(() => {
set(this.delegate, 'playing', 'didStartPlaying');
this.delegate.sendAction('playing', testContext);
expectSendActionDeprecation(() => {
this.runTask(() => {
set(this.delegate, 'playing', 'didStartPlaying');
this.delegate.sendAction('playing', testContext);
});
});

this.assertSendCount(1);
this.assertNamedSendCount('didStartPlaying', 1);
this.assertSentWithArgs([testContext], 'context was sent with the action');

this.runTask(() => {
this.delegate.sendAction('playing', firstContext, secondContext);
expectSendActionDeprecation(() => {
this.runTask(() => {
this.delegate.sendAction('playing', firstContext, secondContext);
});
});

this.assertSendCount(2);
Expand Down Expand Up @@ -262,8 +295,9 @@ moduleFor(
});

this.renderDelegate();

this.runTask(() => innerChild.sendAction('bar', 'something special'));
expectSendActionDeprecation(() => {
this.runTask(() => innerChild.sendAction('bar', 'something special'));
});
}

['@test asserts if called on a destroyed component']() {
Expand Down Expand Up @@ -303,7 +337,7 @@ moduleFor(
["@test sendAction should trigger an action on the parent component's controller if it exists"](
assert
) {
assert.expect(15);
assert.expect(20);

let component;

Expand Down Expand Up @@ -402,34 +436,46 @@ moduleFor(
this.addTemplate('c.e', '{{foo-bar val=".e" poke="poke"}}');

return this.visit('/a')
.then(() => component.sendAction('poke', 'top'))
.then(() => {
expectSendActionDeprecation(() => component.sendAction('poke', 'top'));
})
.then(() => {
this.assertText('a');
return this.visit('/b');
})
.then(() => component.sendAction('poke', 'top no controller'))
.then(() => {
expectSendActionDeprecation(() => component.sendAction('poke', 'top no controller'));
})
.then(() => {
this.assertText('b');
return this.visit('/c');
})
.then(() => component.sendAction('poke', 'top with nested no controller'))
.then(() => {
expectSendActionDeprecation(() => {
component.sendAction('poke', 'top with nested no controller');
});
})
.then(() => {
this.assertText('c');
return this.visit('/c/d');
})
.then(() => component.sendAction('poke', 'nested'))
.then(() => {
expectSendActionDeprecation(() => component.sendAction('poke', 'nested'));
})
.then(() => {
this.assertText('c.d');
return this.visit('/c/e');
})
.then(() => component.sendAction('poke', 'nested no controller'))
.then(() => {
expectSendActionDeprecation(() => component.sendAction('poke', 'nested no controller'));
})
.then(() => this.assertText('c.e'));
}

["@test sendAction should not trigger an action in an outlet's controller if a parent component handles it"](
assert
) {
assert.expect(1);
assert.expect(2);

let component;

Expand Down Expand Up @@ -463,7 +509,9 @@ moduleFor(
})
);

return this.visit('/').then(() => component.sendAction('poke'));
return this.visit('/').then(() => {
expectSendActionDeprecation(() => component.sendAction('poke'));
});
}
}
);
Expand All @@ -472,7 +520,7 @@ moduleFor(
'Components test: sendAction of a closure action',
class extends RenderingTest {
['@test action should be called'](assert) {
assert.expect(1);
assert.expect(2);
let component;

this.registerComponent('inner-component', {
Expand All @@ -495,8 +543,9 @@ moduleFor(
});

this.render('{{outer-component}}');

this.runTask(() => component.sendAction('submitAction'));
expectSendActionDeprecation(() => {
this.runTask(() => component.sendAction('submitAction'));
});
}

['@test contexts passed to sendAction are appended to the bound arguments on a closure action']() {
Expand Down Expand Up @@ -531,8 +580,9 @@ moduleFor(
});

this.render('{{outer-component}}');

this.runTask(() => innerComponent.sendAction('innerSubmit', fourth));
expectSendActionDeprecation(() => {
this.runTask(() => innerComponent.sendAction('innerSubmit', fourth));
});

this.assert.deepEqual(
actualArgs,
Expand Down
4 changes: 3 additions & 1 deletion packages/ember-glimmer/tests/integration/helpers/element-action-test.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1440,7 +1440,9 @@ moduleFor(
let InnerComponent = Component.extend({
click() {
innerClickCalled = true;
this.sendAction();
expectDeprecation(() => {
this.sendAction();
}, /You called (.*).sendAction\((.*)\) but Component#sendAction is deprecated. Please use closure actions instead./);
},
});

Expand Down
Loading

0 comments on commit b21f77b

Please sign in to comment.