Skip to content

Commit

Permalink
Merge pull request #916 from bmish/no-side-effects-event-functions
Browse files Browse the repository at this point in the history 
Add `catchEvents` option (default false) to `no-side-effects` rule
  • Loading branch information
@bmish
bmish committed Aug 18, 2020
2 parents d9107e1 + 157bb0b commit 809456f
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 11 deletions.
6 changes: 6 additions & 0 deletions docs/rules/no-side-effects.md
View file
Expand Up @@ -41,3 +41,9 @@ export default Component.extend({
})
});
```

## Configuration

This rule takes an optional object containing:

* `boolean` -- `catchEvents` -- whether the rule should catch function calls that send actions or events (default `false`, TODO: enable in next major release)
85 changes: 74 additions & 11 deletions lib/rules/no-side-effects.js
View file
Expand Up @@ -21,7 +21,7 @@ function isEmberSetThis(node, importedEmberName) {
types.isIdentifier(node.callee.property) &&
['set', 'setProperties'].includes(node.callee.property.name) &&
node.arguments.length > 0 &&
memberExpressionBeginWithThis(node.arguments[0])
memberExpressionBeginsWithThis(node.arguments[0])
);
}

Expand All @@ -32,7 +32,7 @@ function isImportedSetThis(node, importedSetName, importedSetPropertiesName) {
types.isIdentifier(node.callee) &&
[importedSetName, importedSetPropertiesName].includes(node.callee.name) &&
node.arguments.length > 0 &&
memberExpressionBeginWithThis(node.arguments[0])
memberExpressionBeginsWithThis(node.arguments[0])
);
}

Expand All @@ -44,15 +44,50 @@ function isThisSet(node) {
types.isMemberExpression(node.callee) &&
types.isIdentifier(node.callee.property) &&
['set', 'setProperties'].includes(node.callee.property.name) &&
memberExpressionBeginWithThis(node.callee.object)
memberExpressionBeginsWithThis(node.callee.object)
);
}

function memberExpressionBeginWithThis(node) {
// import { sendEvent } from "@ember/object/events"
// Ember.sendEvent

// Looks for variations like:
// - this.send(...)
// - Ember.send(...)
const DISALLOWED_FUNCTION_CALLS = new Set(['send', 'sendAction', 'sendEvent', 'trigger']);
function isDisallowedFunctionCall(node, importedEmberName) {
return (
types.isCallExpression(node) &&
types.isMemberExpression(node.callee) &&
(types.isThisExpression(node.callee.object) ||
(types.isIdentifier(node.callee.object) && node.callee.object.name === importedEmberName)) &&
types.isIdentifier(node.callee.property) &&
DISALLOWED_FUNCTION_CALLS.has(node.callee.property.name)
);
}

// sendEvent(...)
function isImportedSendEventCall(node, importedSendEventName) {
return (
types.isCallExpression(node) &&
types.isIdentifier(node.callee) &&
node.callee.name === importedSendEventName
);
}

/**
* Finds:
* - this
* - this.foo
* - this.foo.bar
* - this?.foo?.bar
* @param {node} node
*/
function memberExpressionBeginsWithThis(node) {
if (types.isThisExpression(node)) {
return true;
} else if (types.isMemberExpression(node)) {
return memberExpressionBeginWithThis(node.object);
} else if (types.isMemberExpression(node) || types.isOptionalMemberExpression(node)) {
return memberExpressionBeginsWithThis(node.object);
}
return false;
}
Expand All @@ -61,16 +96,20 @@ function memberExpressionBeginWithThis(node) {
* Recursively finds calls that could be side effects in a computed property function body.
*
* @param {ASTNode} computedPropertyBody body of computed property to search
* @param {boolean} catchEvents
* @param {string} importedEmberName
* @param {string} importedSetName
* @param {string} importedSetPropertiesName
* @param {string} importedSendEventName
* @returns {Array<ASTNode>}
*/
function findSideEffects(
computedPropertyBody,
catchEvents,
importedEmberName,
importedSetName,
importedSetPropertiesName
importedSetPropertiesName,
importedSendEventName
) {
const results = [];

Expand All @@ -80,7 +119,9 @@ function findSideEffects(
isEmberSetThis(child, importedEmberName) || // Ember.set(this, 'foo', 123)
isImportedSetThis(child, importedSetName, importedSetPropertiesName) || // set(this, 'foo', 123)
isThisSet(child) || // this.set('foo', 123)
propertySetterUtils.isThisSet(child) // this.foo = 123;
propertySetterUtils.isThisSet(child) || // this.foo = 123;
(catchEvents && isDisallowedFunctionCall(child, importedEmberName)) || // this.send('done')
(catchEvents && isImportedSendEventCall(child, importedSendEventName)) // sendEvent(...)
) {
results.push(child);
}
Expand All @@ -103,16 +144,30 @@ module.exports = {
'https://github.com/ember-cli/eslint-plugin-ember/tree/master/docs/rules/no-side-effects.md',
},
fixable: null,
schema: [],
schema: [
{
type: 'object',
properties: {
catchEvents: {
type: 'boolean',
default: false,
},
},
},
],
},

ERROR_MESSAGE,

create(context) {
// Options:
const catchEvents = context.options[0] && context.options[0].catchEvents;

let importedEmberName;
let importedComputedName;
let importedSetName;
let importedSetPropertiesName;
let importedSendEventName;

const report = function (node) {
context.report(node, ERROR_MESSAGE);
Expand All @@ -131,6 +186,10 @@ module.exports = {
importedSetPropertiesName ||
getImportIdentifier(node, '@ember/object', 'setProperties');
}
if (node.source.value === '@ember/object/events') {
importedSendEventName =
importedSendEventName || getImportIdentifier(node, '@ember/object/events', 'sendEvent');
}
},

CallExpression(node) {
Expand All @@ -142,9 +201,11 @@ module.exports = {

findSideEffects(
computedPropertyBody,
catchEvents,
importedEmberName,
importedSetName,
importedSetPropertiesName
importedSetPropertiesName,
importedSendEventName
).forEach(report);
},

Expand All @@ -157,9 +218,11 @@ module.exports = {

findSideEffects(
computedPropertyBody,
catchEvents,
importedEmberName,
importedSetName,
importedSetPropertiesName
importedSetPropertiesName,
importedSendEventName
).forEach(report);
},
};
Expand Down
85 changes: 85 additions & 0 deletions tests/lib/rules/no-side-effects.js
View file
Expand Up @@ -13,6 +13,7 @@ const { ERROR_MESSAGE } = rule;
// ------------------------------------------------------------------------------

const eslintTester = new RuleTester({
parser: require.resolve('babel-eslint'),
parserOptions: { ecmaVersion: 6, sourceType: 'module' },
});

Expand Down Expand Up @@ -83,6 +84,23 @@ eslintTester.run('no-side-effects', rule, {
"import Ember from 'ember'; Ember.setProperties(this, 'x', 123);",
'this.x = 123;',
'this.x.y = 123;',

// Events (but `catchEvents` option off):
'computed(function() { this.send(); })',
'computed(function() { this.sendAction(); })',
'computed(function() { this.sendEvent(); })',
'computed(function() { this.trigger(); })',
'import { sendEvent } from "@ember/object/events"; computed(function() { sendEvent(); })',

// Not in a computed property (events):
{ code: 'this.send()', options: [{ catchEvents: true }] },
{ code: 'this.sendAction()', options: [{ catchEvents: true }] },
{ code: 'this.sendEvent()', options: [{ catchEvents: true }] },
{ code: 'this.trigger()', options: [{ catchEvents: true }] },
{
code: 'import { sendEvent } from "@ember/object/events"; sendEvent();',
options: [{ catchEvents: true }],
},
].map(addComputedImport),
invalid: [
// this.set
Expand Down Expand Up @@ -134,6 +152,12 @@ eslintTester.run('no-side-effects', rule, {
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code:
'import Ember from "ember"; computed(function() { Ember.set(this.foo?.bar, "testAmount", test.length); return ""; })',
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code:
'import Ember from "ember"; computed(function() { Ember.setProperties(this, "testAmount", test.length); return ""; })',
Expand Down Expand Up @@ -225,5 +249,66 @@ eslintTester.run('no-side-effects', rule, {
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'AssignmentExpression' }],
},

// Events (from this):
{
code: 'computed(function() { this.send(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'computed(function() { this.sendAction(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'computed(function() { this.sendEvent(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'computed(function() { this.trigger(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},

// Events (from Ember):
{
code: 'import Ember from "ember"; computed(function() { Ember.send(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'import Ember from "ember"; computed(function() { Ember.sendAction(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'import Ember from "ember"; computed(function() { Ember.sendEvent(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
{
code: 'import Ember from "ember"; computed(function() { Ember.trigger(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},

{
// Imported sendEvent function:
code:
'import { sendEvent as se } from "@ember/object/events"; computed(function() { se(); })',
options: [{ catchEvents: true }],
output: null,
errors: [{ message: ERROR_MESSAGE, type: 'CallExpression' }],
},
].map(addComputedImport),
});

0 comments on commit 809456f

Please sign in to comment.