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.

Sign up for GitHub

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

Jump to bottom

Add catchEvents option (default false) to no-side-effects rule #916

Merged
merged 1 commit into from Aug 18, 2020
Merged

Add catchEvents option (default false) to no-side-effects rule #916

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions docs/rules/no-side-effects.md
View file
Open in desktop
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
Open in desktop
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:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this also find this.foo?.bar?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess so, updated.

* - 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
Open in desktop
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),
});