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

[NewDot API] Add No api side effect actions rule #53

Merged
merged 16 commits into from
Jun 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions eslint-plugin-expensify/CONST.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,8 @@ module.exports = {
PREFER_STR_METHOD: 'Prefer \'Str.{{method}}\' over the native function.',

// eslint-disable-next-line max-len
NO_MULTIPLE_API_CALLS: 'Do not call API or DeprecatedAPI multiple times in the same method. The API response should return all the necessary data in a single request, and API calls should not be chained together.',

NO_MULTIPLE_API_CALLS: 'Do not call API multiple times in the same method. The API response should return all the necessary data in a single request, and API calls should not be chained together.',
NO_CALL_ACTIONS_FROM_ACTIONS: 'Calling actions from inside other actions is forbidden. If an action needs to call another action combine the two actions into a singular API call instead.',
marcaaron marked this conversation as resolved.
Show resolved Hide resolved
NO_API_SIDE_EFFECTS_METHOD: 'Do not use makeRequestWithSideEffects.',
},
};
53 changes: 53 additions & 0 deletions eslint-plugin-expensify/no-call-actions-from-actions.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
const _ = require('underscore');
const lodashGet = require('lodash/get');
const path = require('path');
const {isInActionFile} = require('./utils');
const message = require('./CONST').MESSAGE.NO_CALL_ACTIONS_FROM_ACTIONS;

module.exports = {
create(context) {
let actions = [];

function hasActionCall(tokens) {
return _.find(tokens, (token) => {
return _.includes(actions, token.value);
});
}

function hasAPICall(tokens) {
return _.find(tokens, (token) => {
return token.value === 'API';
});
}

function checkFunctionBody(node) {
if (!isInActionFile(context.getFilename())) {
return;
}

const tokens = context.getSourceCode().getTokens(node);

if (hasAPICall(tokens) && hasActionCall(tokens)) {
context.report({
node,
message,
});
}
}

return {
ImportDeclaration: function(node) {
const pathName = path.resolve(lodashGet(node, 'source.value'));
if (!pathName || !pathName.includes('/actions/')) {
return;
}

const filename = _.last(pathName.split('/'));
actions.push(_.first(filename.split('.')));
},
FunctionDeclaration: checkFunctionBody,
FunctionExpression: checkFunctionBody,
ArrowFunctionExpression: checkFunctionBody,
};
},
};
2 changes: 1 addition & 1 deletion eslint-plugin-expensify/no-multiple-api-calls.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ module.exports = {
let hasCalledAPI = false;

_.each(tokens, (token) => {
const isAPICall = token.value === 'DeprecatedAPI' || token.value === 'API';
const isAPICall = token.value === 'API';

if (!isAPICall) {
return;
Expand Down
90 changes: 90 additions & 0 deletions eslint-plugin-expensify/tests/no-call-actions-from-actions.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
const RuleTester = require('eslint').RuleTester;
const rule = require('../no-call-actions-from-actions');
const message = require('../CONST').MESSAGE.NO_CALL_ACTIONS_FROM_ACTIONS;

const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
},
});

ruleTester.run('no-call-actions-from-actions', rule, {
valid: [
{
// Making one API call should be valid
code: `
function test() {
API.write('Report_AddComment', params);
}
`,
filename: './src/libs/actions/Test.js',
},
{
// Calling DeprecatedAPI should be valid as we focus on rewriting the new API using this best practice
code: `
function test() {
DeprecatedAPI.read('Report_AddComment', params).then((value) => {
Action(params);
});
}
`,
filename: './src/libs/actions/Test.js',
},
{
// Having two functions in one file, each with one API or Action call should be valid
code: `
import Action from './src/libs/actions/Action.js'
function test() {
API.write('Report_AddComment', params);
}
function test2() {
Action(params);
}
`,
filename: './src/libs/actions/Test.js',
},
{
// Make sure that we will not test a file which is not in '/actions/'
code: `
import Action from './src/libs/actions/Action.js'
function test() {
API.write('Report_AddComment', params);
Action(params);
}
`,
filename: './src/libs/notActions/Test.js',
},
],
invalid: [
{
// Making an API call and calling an Action should be invalid
code: `
import Action from './src/libs/actions/Action.js'
function test() {
API.write('Report_AddComment', params);
Action(params);
}
`,
errors: [{
message,
}],
filename: './src/libs/actions/Test.js',
},
{
// Make sure chaining the API/Action calls is also invalid
code: `
import Action from './src/libs/actions/Action.js'
function test() {
API.read('Report_AddComment', params).then((value) => {
Action(params);
});
}
`,
errors: [{
message,
}],
filename: './src/libs/actions/Test.js',
},
],
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ ruleTester.run('no-multiple-api-calls', rule, {
{
code: `
function test() {
DeprecatedAPI.CreateLogin(params).then(res => API.write('PreferredLocale_Update', params2));
API.CreateLogin(params).then(res => API.write('PreferredLocale_Update', params2));
}
`,
errors: [{
Expand Down
2 changes: 1 addition & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "eslint-config-expensify",
"version": "2.0.28",
"version": "2.0.29",
"description": "Expensify's ESLint configuration following our style guide",
"main": "index.js",
"repository": {
Expand Down
1 change: 1 addition & 0 deletions rules/expensify.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ module.exports = {
'rulesdir/no-useless-compose': 'error',
'rulesdir/prefer-import-module-contents': 'error',
'rulesdir/no-multiple-api-calls': 'error',
'rulesdir/no-call-actions-from-actions': 'error',
'rulesdir/no-api-side-effects-method': 'error',
'no-restricted-imports': ['error', {
paths: [{
Expand Down