Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FEAT] assert that actions cannot be sent from a destroyed/destroying object #16387

Merged
merged 2 commits into from
Mar 22, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { assign } from 'ember-utils';
import { runDestroy } from "internal-test-helpers";
import {
moduleFor,
RenderingTest,
Expand Down Expand Up @@ -259,6 +260,34 @@ moduleFor('Components test: sendAction', class extends RenderingTest {

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

['@test asserts if called on a destroyed component']() {
let component;

this.registerComponent('rip-alley', {
ComponentClass: Component.extend({
init() {
this._super();
component = this;
},

toString() {
return 'component:rip-alley';
}
})
});

this.render('{{rip-alley}}');

runDestroy(component);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d vaguely prefer a more natural way to destroy the component, e.g. render it in an if and toggle the condition

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rwjblue didn't think of that, but I agree sticking closer to real world is best. Will update to that. Do we have a helper or do I need to wrap a set in a run?


expectAssertion(
() => {
component.sendAction('trigger-me-dead');
},
"Attempted to call .sendAction() with the action 'trigger-me-dead' on the destroyed object 'component:rip-alley'."
);
}
});

moduleFor('Components test: sendAction to a controller', class extends ApplicationTest {
Expand Down Expand Up @@ -617,4 +646,32 @@ moduleFor('Components test: send', class extends RenderingTest {
actions: ['foo']
});
}

['@test asserts if called on a destroyed component']() {
let component;

this.registerComponent('rip-alley', {
ComponentClass: Component.extend({
init() {
this._super();
component = this;
},

toString() {
return 'component:rip-alley';
}
})
});

this.render('{{rip-alley}}');

runDestroy(component);

expectAssertion(
() => {
component.send('trigger-me-dead');
},
"Attempted to call .send() with the action 'trigger-me-dead' on the destroyed object 'component:rip-alley'."
);
}
});
4 changes: 4 additions & 0 deletions packages/ember-routing/lib/system/route.js
Original file line number Diff line number Diff line change
Expand Up @@ -1272,6 +1272,10 @@ let Route = EmberObject.extend(ActionHandler, Evented, {
@public
*/
send(...args) {
assert(
`Attempted to call .send() with the action '${args[0]}' on the destroyed route '${this.routeName}'.`,
!this.isDestroying && !this.isDestroyed
);
if ((this._router && this._router._routerMicrolib) || !isTesting()) {
this._router.send(...args);
} else {
Expand Down
12 changes: 12 additions & 0 deletions packages/ember-routing/tests/system/route_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,18 @@ moduleFor('Route', class extends AbstractTestCase {
assert.equal(false, route.send('returnsFalse'));
assert.equal(undefined, route.send('nonexistent', 1, 2, 3));
}

['@test .send asserts if called on a destroyed route']() {
route.routeName = 'rip-alley';
runDestroy(route);

expectAssertion(
() => {
route.send('trigger-me-dead');
},
"Attempted to call .send() with the action 'trigger-me-dead' on the destroyed route 'rip-alley'."
);
}
});

moduleFor('Route serialize', class extends AbstractTestCase {
Expand Down
4 changes: 4 additions & 0 deletions packages/ember-runtime/lib/mixins/action_handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,10 @@ const ActionHandler = Mixin.create({
@public
*/
send(actionName, ...args) {
assert(
`Attempted to call .send() with the action '${actionName}' on the destroyed object '${this}'.`,
!this.isDestroying && !this.isDestroyed
);
if (this.actions && this.actions[actionName]) {
let shouldBubble = this.actions[actionName].apply(this, args) === true;
if (!shouldBubble) { return; }
Expand Down
22 changes: 21 additions & 1 deletion packages/ember-runtime/tests/controllers/controller_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import Service from '../../system/service';
import { Mixin, get } from 'ember-metal';
import EmberObject from '../../system/object';
import inject from '../../inject';
import { buildOwner } from 'internal-test-helpers';
import { runDestroy, buildOwner } from 'internal-test-helpers';

QUnit.module('Controller event handling');

Expand Down Expand Up @@ -82,6 +82,26 @@ QUnit.test('Action can be handled by a superclass\' actions object', function(as
controller.send('baz');
});

QUnit.test('.send asserts if called on a destroyed controller', function() {
let owner = buildOwner();

owner.register('controller:application', Controller.extend({
toString() {
return 'controller:rip-alley';
}
}));

let controller = owner.lookup('controller:application');
runDestroy(owner);

expectAssertion(
() => {
controller.send('trigger-me-dead');
},
"Attempted to call .send() with the action 'trigger-me-dead' on the destroyed object 'controller:rip-alley'."
);
});

QUnit.module('Controller deprecations');

QUnit.module('Controller Content -> Model Alias');
Expand Down
10 changes: 10 additions & 0 deletions packages/ember-views/lib/mixins/action_support.js
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,11 @@ export default Mixin.create({
@public
*/
sendAction(action, ...contexts) {
assert(
`Attempted to call .sendAction() with the action '${action}' on the destroyed object '${this}'.`,
!this.isDestroying && !this.isDestroyed
);

let actionName;

// Send the default action
Expand All @@ -132,6 +137,11 @@ export default Mixin.create({
},

send(actionName, ...args) {
assert(
`Attempted to call .send() with the action '${actionName}' on the destroyed object '${this}'.`,
!this.isDestroying && !this.isDestroyed
);

let action = this.actions && this.actions[actionName];

if (action) {
Expand Down