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

fix(no-nesting): nested references vars in closure #361

Merged
merged 1 commit into from Sep 29, 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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 30 additions & 0 deletions __tests__/no-nesting.js
Expand Up @@ -45,6 +45,17 @@ ruleTester.run('no-nesting', rule, {
'doThing().then(function() { return Promise.resolve(4) })',
'doThing().then(() => Promise.resolve(4))',
'doThing().then(() => Promise.all([a]))',

// references vars in closure
`doThing()
.then(a => getB(a)
.then(b => getC(a, b))
)`,
`doThing()
.then(a => {
const c = a * 2;
return getB(c).then(b => getC(c, b))
})`,
],

invalid: [
Expand Down Expand Up @@ -80,5 +91,24 @@ ruleTester.run('no-nesting', rule, {
code: 'doThing().then(() => b.catch())',
errors: [{ message: errorMessage }],
},
// references vars in closure
{
code: `
doThing()
.then(a => getB(a)
.then(b => getC(b))
)`,
errors: [{ message: errorMessage, line: 4 }],
},
{
code: `
doThing()
.then(a => getB(a)
.then(b => getC(a, b)
.then(c => getD(a, c))
)
)`,
errors: [{ message: errorMessage, line: 5 }],
},
],
})
20 changes: 20 additions & 0 deletions rules/lib/has-promise-callback.js
Expand Up @@ -6,6 +6,26 @@

'use strict'

/**
* @typedef {import('estree').SimpleCallExpression} CallExpression
* @typedef {import('estree').MemberExpression} MemberExpression
* @typedef {import('estree').Identifier} Identifier
*
* @typedef {object} NameIsThenOrCatch
* @property {'then' | 'catch'} name
*
* @typedef {object} PropertyIsThenOrCatch
* @property {Identifier & NameIsThenOrCatch} property
*
* @typedef {object} CalleeIsPromiseCallback
* @property {MemberExpression & PropertyIsThenOrCatch} callee
*
* @typedef {CallExpression & CalleeIsPromiseCallback} HasPromiseCallback
*/
/**
* @param {import('estree').Node} node
* @returns {node is HasPromiseCallback}
*/
function hasPromiseCallback(node) {
// istanbul ignore if -- only being called within `CallExpression`
if (node.type !== 'CallExpression') return
Expand Down
92 changes: 90 additions & 2 deletions rules/no-nesting.js
Expand Up @@ -18,12 +18,100 @@ module.exports = {
schema: [],
},
create(context) {
/**
* Array of callback function scopes.
* Scopes are in order closest to the current node.
* @type {import('eslint').Scope.Scope[]}
*/
const callbackScopes = []

/**
* @param {import('eslint').Scope.Scope} scope
* @returns {Iterable<import('eslint').Scope.Reference>}
*/
function* iterateDefinedReferences(scope) {
for (const variable of scope.variables) {
for (const reference of variable.references) {
yield reference
}
}
}

return {
':function'(node) {
if (isInsidePromise(node)) {
callbackScopes.unshift(context.getScope())
}
},
':function:exit'(node) {
if (isInsidePromise(node)) {
callbackScopes.shift()
}
},
CallExpression(node) {
if (!hasPromiseCallback(node)) return
if (context.getAncestors().some(isInsidePromise)) {
context.report({ node, message: 'Avoid nesting promises.' })
if (!callbackScopes.length) {
// The node is not in the callback function.
return
}

// Checks if the argument callback uses variables defined in the closest callback function scope.
//
// e.g.
// ```
// doThing()
// .then(a => getB(a)
// .then(b => getC(a, b))
// )
// ```
//
// In the above case, Since the variables it references are undef,
// we cannot refactor the nesting like following:
// ```
// doThing()
// .then(a => getB(a))
// .then(b => getC(a, b))
// ```
//
// However, `getD` can be refactored in the following:
// ```
// doThing()
// .then(a => getB(a)
// .then(b => getC(a, b)
// .then(c => getD(a, c))
// )
// )
// ```
// ↓
// ```
// doThing()
// .then(a => getB(a)
// .then(b => getC(a, b))
// .then(c => getD(a, c))
// )
// ```
// This is why we only check the closest callback function scope.
//
const closestCallbackScope = callbackScopes[0]
for (const reference of iterateDefinedReferences(
closestCallbackScope
)) {
if (
node.arguments.some(
(arg) =>
arg.range[0] <= reference.identifier.range[0] &&
reference.identifier.range[1] <= arg.range[1]
)
) {
// Argument callbacks refer to variables defined in the callback function.
return
}
}

context.report({
node: node.callee.property,
message: 'Avoid nesting promises.',
})
},
}
},
Expand Down