-
Notifications
You must be signed in to change notification settings - Fork 144
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
Move rules settings to ESLint shared config: refactor no-container rule #295
Changes from all commits
0175206
bf11503
585b0e4
81cfde8
ddef4db
a860221
574d062
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -477,31 +477,6 @@ export function isRenderFunction( | |
}); | ||
} | ||
|
||
// TODO: should be removed after v4 is finished | ||
export function isRenderVariableDeclarator( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Removed finally. A better equivalent has been created in |
||
node: TSESTree.VariableDeclarator, | ||
renderFunctions: string[] | ||
): boolean { | ||
if (node.init) { | ||
if (ASTUtils.isAwaitExpression(node.init)) { | ||
return ( | ||
node.init.argument && | ||
isRenderFunction( | ||
node.init.argument as TSESTree.CallExpression, | ||
renderFunctions | ||
) | ||
); | ||
} else { | ||
return ( | ||
isCallExpression(node.init) && | ||
isRenderFunction(node.init, renderFunctions) | ||
); | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
|
||
// TODO: extract into types file? | ||
export type ImportModuleNode = | ||
| TSESTree.ImportDeclaration | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,21 +1,19 @@ | ||
import { ASTUtils, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { | ||
ESLintUtils, | ||
TSESTree, | ||
ASTUtils, | ||
} from '@typescript-eslint/experimental-utils'; | ||
import { getDocsUrl } from '../utils'; | ||
import { | ||
getDeepestIdentifierNode, | ||
getFunctionName, | ||
getInnermostReturningFunction, | ||
isMemberExpression, | ||
isObjectPattern, | ||
isProperty, | ||
isRenderVariableDeclarator, | ||
} from '../node-utils'; | ||
import { createTestingLibraryRule } from '../create-testing-library-rule'; | ||
|
||
export const RULE_NAME = 'no-container'; | ||
export type MessageIds = 'noContainer'; | ||
type Options = [{ renderFunctions?: string[] }]; | ||
type Options = []; | ||
|
||
export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | ||
export default createTestingLibraryRule<Options, MessageIds>({ | ||
name: RULE_NAME, | ||
meta: { | ||
type: 'problem', | ||
|
@@ -29,48 +27,52 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | |
'Avoid using container methods. Prefer using the methods from Testing Library, such as "getByRole()"', | ||
}, | ||
fixable: null, | ||
schema: [ | ||
{ | ||
type: 'object', | ||
properties: { | ||
renderFunctions: { | ||
type: 'array', | ||
}, | ||
}, | ||
}, | ||
], | ||
schema: [], | ||
}, | ||
defaultOptions: [ | ||
{ | ||
renderFunctions: [], | ||
}, | ||
], | ||
defaultOptions: [], | ||
|
||
create(context, [options]) { | ||
const { renderFunctions } = options; | ||
create(context, [], helpers) { | ||
const destructuredContainerPropNames: string[] = []; | ||
let renderWrapperName: string = null; | ||
const renderWrapperNames: string[] = []; | ||
let renderResultVarName: string = null; | ||
let containerName: string = null; | ||
let containerCallsMethod = false; | ||
|
||
function detectRenderWrapper(node: TSESTree.Identifier): void { | ||
const innerFunction = getInnermostReturningFunction(context, node); | ||
|
||
if (innerFunction) { | ||
renderWrapperNames.push(getFunctionName(innerFunction)); | ||
} | ||
} | ||
|
||
function showErrorIfChainedContainerMethod( | ||
innerNode: TSESTree.MemberExpression | ||
) { | ||
if (isMemberExpression(innerNode)) { | ||
if (ASTUtils.isIdentifier(innerNode.object)) { | ||
const isContainerName = innerNode.object.name === containerName; | ||
const isRenderWrapper = innerNode.object.name === renderWrapperName; | ||
|
||
if (isContainerName) { | ||
context.report({ | ||
node: innerNode, | ||
messageId: 'noContainer', | ||
}); | ||
return; | ||
} | ||
|
||
const isRenderWrapper = innerNode.object.name === renderResultVarName; | ||
containerCallsMethod = | ||
ASTUtils.isIdentifier(innerNode.property) && | ||
innerNode.property.name === 'container' && | ||
isRenderWrapper; | ||
|
||
if (isContainerName || containerCallsMethod) { | ||
if (containerCallsMethod) { | ||
context.report({ | ||
node: innerNode, | ||
node: innerNode.property, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If |
||
messageId: 'noContainer', | ||
}); | ||
return; | ||
} | ||
} | ||
showErrorIfChainedContainerMethod( | ||
|
@@ -80,35 +82,12 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | |
} | ||
|
||
return { | ||
VariableDeclarator(node) { | ||
if (isRenderVariableDeclarator(node, ['render', ...renderFunctions])) { | ||
if (isObjectPattern(node.id)) { | ||
const containerIndex = node.id.properties.findIndex( | ||
(property) => | ||
isProperty(property) && | ||
ASTUtils.isIdentifier(property.key) && | ||
property.key.name === 'container' | ||
); | ||
const nodeValue = | ||
containerIndex !== -1 && node.id.properties[containerIndex].value; | ||
if (ASTUtils.isIdentifier(nodeValue)) { | ||
containerName = nodeValue.name; | ||
} else { | ||
isObjectPattern(nodeValue) && | ||
nodeValue.properties.forEach( | ||
(property) => | ||
isProperty(property) && | ||
ASTUtils.isIdentifier(property.key) && | ||
destructuredContainerPropNames.push(property.key.name) | ||
); | ||
} | ||
} else { | ||
renderWrapperName = ASTUtils.isIdentifier(node.id) && node.id.name; | ||
} | ||
CallExpression(node) { | ||
const callExpressionIdentifier = getDeepestIdentifierNode(node); | ||
if (helpers.isRenderUtil(callExpressionIdentifier)) { | ||
detectRenderWrapper(callExpressionIdentifier); | ||
} | ||
}, | ||
|
||
CallExpression(node) { | ||
if (isMemberExpression(node.callee)) { | ||
showErrorIfChainedContainerMethod(node.callee); | ||
} else { | ||
|
@@ -120,6 +99,47 @@ export default ESLintUtils.RuleCreator(getDocsUrl)<Options, MessageIds>({ | |
}); | ||
} | ||
}, | ||
|
||
VariableDeclarator(node) { | ||
const initIdentifierNode = getDeepestIdentifierNode(node.init); | ||
|
||
const isRenderWrapperVariableDeclarator = initIdentifierNode | ||
? renderWrapperNames.includes(initIdentifierNode.name) | ||
: false; | ||
|
||
if ( | ||
!helpers.isRenderVariableDeclarator(node) && | ||
!isRenderWrapperVariableDeclarator | ||
) { | ||
return; | ||
} | ||
Comment on lines
+106
to
+115
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rule implementation is mostly the same, except this bit to detect reportable |
||
|
||
if (isObjectPattern(node.id)) { | ||
const containerIndex = node.id.properties.findIndex( | ||
(property) => | ||
isProperty(property) && | ||
ASTUtils.isIdentifier(property.key) && | ||
property.key.name === 'container' | ||
); | ||
|
||
const nodeValue = | ||
containerIndex !== -1 && node.id.properties[containerIndex].value; | ||
|
||
if (ASTUtils.isIdentifier(nodeValue)) { | ||
containerName = nodeValue.name; | ||
} else { | ||
isObjectPattern(nodeValue) && | ||
nodeValue.properties.forEach( | ||
(property) => | ||
isProperty(property) && | ||
ASTUtils.isIdentifier(property.key) && | ||
destructuredContainerPropNames.push(property.key.name) | ||
); | ||
} | ||
} else { | ||
renderResultVarName = ASTUtils.isIdentifier(node.id) && node.id.name; | ||
} | ||
}, | ||
}; | ||
}, | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,7 @@ | ||
import { | ||
getDeepestIdentifierNode, | ||
getFunctionName, | ||
getInnermostReturningFunction, | ||
getPropertyIdentifierNode, | ||
getReferenceNode, | ||
isObjectPattern, | ||
|
@@ -41,12 +43,28 @@ export default createTestingLibraryRule<Options, MessageIds>({ | |
create(context, [], helpers) { | ||
const suspiciousDebugVariableNames: string[] = []; | ||
const suspiciousReferenceNodes: TSESTree.Identifier[] = []; | ||
const renderWrapperNames: string[] = []; | ||
|
||
function detectRenderWrapper(node: TSESTree.Identifier): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I forgot to detect this properly in the previous PR, so I added it here. |
||
const innerFunction = getInnermostReturningFunction(context, node); | ||
|
||
if (innerFunction) { | ||
renderWrapperNames.push(getFunctionName(innerFunction)); | ||
} | ||
} | ||
|
||
return { | ||
VariableDeclarator(node) { | ||
const initIdentifierNode = getDeepestIdentifierNode(node.init); | ||
|
||
if (!helpers.isRenderUtil(initIdentifierNode)) { | ||
const isRenderWrapperVariableDeclarator = initIdentifierNode | ||
? renderWrapperNames.includes(initIdentifierNode.name) | ||
: false; | ||
|
||
if ( | ||
!helpers.isRenderVariableDeclarator(node) && | ||
!isRenderWrapperVariableDeclarator | ||
) { | ||
return; | ||
} | ||
|
||
|
@@ -74,6 +92,10 @@ export default createTestingLibraryRule<Options, MessageIds>({ | |
}, | ||
CallExpression(node) { | ||
const callExpressionIdentifier = getDeepestIdentifierNode(node); | ||
if (helpers.isRenderUtil(callExpressionIdentifier)) { | ||
detectRenderWrapper(callExpressionIdentifier); | ||
} | ||
|
||
const referenceNode = getReferenceNode(node); | ||
const referenceIdentifier = getPropertyIdentifierNode(referenceNode); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Safety check. Apparently there was some place calling this method with
null
node. I need to investigate this properly, but I did this workaround meanwhile.