From 9ebf44edf5acf96f6923cd62af31e5421994a7e0 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Sun, 28 Jun 2020 18:48:42 -0300 Subject: [PATCH 01/22] refactor(utils): add properties and methods that returns another Node --- lib/utils.ts | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/lib/utils.ts b/lib/utils.ts index 2652aa08..b4f094fb 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -63,6 +63,27 @@ const ASYNC_UTILS = [ 'waitForDomChange', ]; +const PROPERTIES_RETURNING_NODES = [ + 'firstChild', + 'lastChild', + 'nextElementSibling', + 'nextSibling', + 'parentNode', + 'previousElementSibling', + 'previousSibling', + 'rootNode', +]; + +const METHODS_RETURNING_NODES = [ + 'closest', + 'getElementsByClassName', + 'getElementsByTagName', + 'getElementsByTagNameNS', + 'querySelector', + 'querySelectorAll', + '' +]; + export { getDocsUrl, SYNC_QUERIES_VARIANTS, @@ -74,4 +95,6 @@ export { ALL_QUERIES_COMBINATIONS, ASYNC_UTILS, LIBRARY_MODULES, + PROPERTIES_RETURNING_NODES, + METHODS_RETURNING_NODES, }; From 4d41edca3c66ceb6ecfa23ade1ce7f522fa05178 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Sun, 28 Jun 2020 18:49:52 -0300 Subject: [PATCH 02/22] test(no-node-access): add first scenarios --- tests/lib/rules/no-node-access.test.ts | 53 ++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 tests/lib/rules/no-node-access.test.ts diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts new file mode 100644 index 00000000..97c22243 --- /dev/null +++ b/tests/lib/rules/no-node-access.test.ts @@ -0,0 +1,53 @@ +import { createRuleTester } from '../test-utils'; +import rule, { RULE_NAME } from '../../../lib/rules/no-node-access'; + +const ruleTester = createRuleTester({ + ecmaFeatures: { + jsx: true, + }, +}); + +ruleTester.run(RULE_NAME, rule, { + valid: [ + { + code: ` + const buttonText = screen.getByText('submit'); + `, + }, + { + code: ` + const obj = { + firstChild:
child
+ } + obj.firstChild + `, + }, + ], + invalid: [ + { + code: ` + function getExampleDOM() { + const container = document.createElement('div'); + container.innerHTML = \` + + + + + + + + \`; + return container; + } + const exampleDOM = getExampleDOM(); + const buttons = screen.getAllByRole(exampleDOM, 'button'); + const buttonText = buttons[1].firstChild + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, + ], +}); From 9ebf3d2967ab75ec99eb4cbef954f70a5111924b Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Sun, 28 Jun 2020 20:20:55 -0300 Subject: [PATCH 03/22] feat(no-node-access): add rule with few test cases --- lib/index.ts | 5 ++ lib/rules/no-node-access.ts | 82 ++++++++++++++++++++++++++ tests/__snapshots__/index.test.ts.snap | 3 + tests/lib/rules/no-node-access.test.ts | 39 +++++++++--- 4 files changed, 122 insertions(+), 7 deletions(-) create mode 100644 lib/rules/no-node-access.ts diff --git a/lib/index.ts b/lib/index.ts index d9ea7a77..d26d37a7 100644 --- a/lib/index.ts +++ b/lib/index.ts @@ -7,6 +7,7 @@ import noContainer from './rules/no-container'; import noDebug from './rules/no-debug'; import noDomImport from './rules/no-dom-import'; import noManualCleanup from './rules/no-manual-cleanup'; +import noNodeAccess from './rules/no-node-access'; import noWaitForEmptyCallback from './rules/no-wait-for-empty-callback'; import noPromiseInFireEvent from './rules/no-promise-in-fire-event'; import preferExplicitAssert from './rules/prefer-explicit-assert'; @@ -25,6 +26,7 @@ const rules = { 'no-debug': noDebug, 'no-dom-import': noDomImport, 'no-manual-cleanup': noManualCleanup, + 'no-node-access': noNodeAccess, 'no-promise-in-fire-event': noPromiseInFireEvent, 'no-wait-for-empty-callback': noWaitForEmptyCallback, 'prefer-explicit-assert': preferExplicitAssert, @@ -49,6 +51,7 @@ const angularRules = { 'testing-library/no-container': 'error', 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'angular'], + 'testing-library/no-node-access': 'error', }; const reactRules = { @@ -56,6 +59,7 @@ const reactRules = { 'testing-library/no-container': 'error', 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'react'], + 'testing-library/no-node-access': 'error', }; const vueRules = { @@ -64,6 +68,7 @@ const vueRules = { 'testing-library/no-container': 'error', 'testing-library/no-debug': 'warn', 'testing-library/no-dom-import': ['error', 'vue'], + 'testing-library/no-node-access': 'error', }; export = { diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts new file mode 100644 index 00000000..f76beded --- /dev/null +++ b/lib/rules/no-node-access.ts @@ -0,0 +1,82 @@ +import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; +import { isIdentifier, isMemberExpression, isLiteral } from '../node-utils'; +import { + getDocsUrl, + ALL_QUERIES_METHODS, + PROPERTIES_RETURNING_NODES, + METHODS_RETURNING_NODES, +} from '../utils'; + +export const RULE_NAME = 'no-node-access'; + +const ALL_RETURNING_NODES = [ + ...PROPERTIES_RETURNING_NODES, + ...METHODS_RETURNING_NODES, +]; + +export default ESLintUtils.RuleCreator(getDocsUrl)({ + name: RULE_NAME, + meta: { + type: 'problem', + docs: { + description: 'Disallow the use of Node methods', + category: 'Best Practices', + recommended: 'error', + }, + messages: { + noNodeAccess: + 'Avoid direct Node access. Prefer using the methods from Testing Library."', + }, + fixable: null, + schema: [], + }, + defaultOptions: [], + + create(context) { + const variablesWithNodes: string[] = []; + + function identifyVariablesWithNodes(node: TSESTree.MemberExpression) { + const methodCalled = ALL_QUERIES_METHODS.filter( + methodName => + isIdentifier(node.property) && node.property.name.includes(methodName) + ); + const returnsNodeElement = Boolean(methodCalled.length); + + const callExpression = node.parent as TSESTree.CallExpression; + const variableDeclarator = callExpression.parent as TSESTree.VariableDeclarator; + const variableName = + isIdentifier(variableDeclarator.id) && variableDeclarator.id.name; + + if (returnsNodeElement) { + variablesWithNodes.push(variableName); + } + } + + function showErrorForNodeAccess(node: TSESTree.Identifier) { + if (variablesWithNodes.includes(node.name)) { + if ( + isMemberExpression(node.parent) && + isLiteral(node.parent.property) && + typeof node.parent.property.value === 'number' + ) { + context.report({ + node: node, + messageId: 'noNodeAccess', + }); + } + isMemberExpression(node.parent) && + isIdentifier(node.parent.property) && + ALL_RETURNING_NODES.includes(node.parent.property.name) && + context.report({ + node: node, + messageId: 'noNodeAccess', + }); + } + } + + return { + ['VariableDeclarator > CallExpression > MemberExpression']: identifyVariablesWithNodes, + ['MemberExpression > Identifier']: showErrorForNodeAccess, + }; + }, +}); diff --git a/tests/__snapshots__/index.test.ts.snap b/tests/__snapshots__/index.test.ts.snap index b86f6a5c..250540c3 100644 --- a/tests/__snapshots__/index.test.ts.snap +++ b/tests/__snapshots__/index.test.ts.snap @@ -15,6 +15,7 @@ Object { "error", "angular", ], + "testing-library/no-node-access": "error", "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", @@ -55,6 +56,7 @@ Object { "error", "react", ], + "testing-library/no-node-access": "error", "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", @@ -79,6 +81,7 @@ Object { "error", "vue", ], + "testing-library/no-node-access": "error", "testing-library/no-promise-in-fire-event": "error", "testing-library/no-wait-for-empty-callback": "error", "testing-library/prefer-find-by": "error", diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index 97c22243..f974663c 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -11,16 +11,16 @@ ruleTester.run(RULE_NAME, rule, { valid: [ { code: ` - const buttonText = screen.getByText('submit'); - `, + const buttonText = screen.getByText('submit'); + `, }, { code: ` - const obj = { - firstChild:
child
- } - obj.firstChild - `, + const obj = { + firstChild:
child
+ } + obj.firstChild + `, }, ], invalid: [ @@ -49,5 +49,30 @@ ruleTester.run(RULE_NAME, rule, { }, ], }, + { + code: ` + function getExampleDOM() { + const container = document.createElement('div'); + container.innerHTML = \` + + + + + + + + \`; + return container; + } + const exampleDOM = getExampleDOM(); + const submitButton = screen.getByText(exampleDOM, 'Submit'); + const previousSibling = submitButton.previousSibling + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, ], }); From cba1f13e5b645cd6468f2939c10562938079864f Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Sun, 28 Jun 2020 20:35:47 -0300 Subject: [PATCH 04/22] test(no-node-access): add scenarios --- tests/lib/rules/no-node-access.test.ts | 53 ++++++++++++++++++++++---- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index f974663c..7f703a61 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -11,19 +11,58 @@ ruleTester.run(RULE_NAME, rule, { valid: [ { code: ` - const buttonText = screen.getByText('submit'); - `, + const buttonText = screen.getByText('submit'); + `, }, { code: ` - const obj = { - firstChild:
child
- } - obj.firstChild - `, + const myObj = { + firstChild: 'Some custom text' + } + const text = myObj.firstChild + `, }, ], invalid: [ + { + code: ` + const buttons = screen.getAllByRole('button'); + const buttonA = buttons[1]; + expect(buttonA.lastChild).toBeInTheDocument(); + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + const buttons = screen.getAllByRole('button'); + buttons[0].firstChild; + const buttonA = buttons[1]; + expect(buttonA.lastChild).toBeInTheDocument(); + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + { + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + const buttonText = screen.getByText('submit'); + const button = buttonText.closest('button'); + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, { code: ` function getExampleDOM() { From 509288901a2c117b6be7069a7682303820824457 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 14:03:27 -0300 Subject: [PATCH 05/22] refactor(no-node-access): simplify conditions --- lib/rules/no-node-access.ts | 52 +++++++++++++++++-------------------- 1 file changed, 24 insertions(+), 28 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index f76beded..09e8519a 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -19,13 +19,13 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ meta: { type: 'problem', docs: { - description: 'Disallow the use of Node methods', + description: 'Disallow the use of methods for direct Node access.', category: 'Best Practices', recommended: 'error', }, messages: { noNodeAccess: - 'Avoid direct Node access. Prefer using the methods from Testing Library."', + 'Avoid direct Node access. Prefer using the methods from Testing Library.', }, fixable: null, schema: [], @@ -36,41 +36,37 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ const variablesWithNodes: string[] = []; function identifyVariablesWithNodes(node: TSESTree.MemberExpression) { - const methodCalled = ALL_QUERIES_METHODS.filter( - methodName => - isIdentifier(node.property) && node.property.name.includes(methodName) - ); - const returnsNodeElement = Boolean(methodCalled.length); - const callExpression = node.parent as TSESTree.CallExpression; const variableDeclarator = callExpression.parent as TSESTree.VariableDeclarator; - const variableName = - isIdentifier(variableDeclarator.id) && variableDeclarator.id.name; + const methodsNames = ALL_QUERIES_METHODS.filter( + method => + isIdentifier(node.property) && node.property.name.includes(method) + ); - if (returnsNodeElement) { - variablesWithNodes.push(variableName); + if (methodsNames.length) { + variablesWithNodes.push( + isIdentifier(variableDeclarator.id) && variableDeclarator.id.name + ); } } function showErrorForNodeAccess(node: TSESTree.Identifier) { if (variablesWithNodes.includes(node.name)) { - if ( - isMemberExpression(node.parent) && - isLiteral(node.parent.property) && - typeof node.parent.property.value === 'number' - ) { - context.report({ - node: node, - messageId: 'noNodeAccess', - }); + if (isMemberExpression(node.parent)) { + const isLiteralNumber = + isLiteral(node.parent.property) && + typeof node.parent.property.value === 'number'; + const hasForbiddenMethod = + isIdentifier(node.parent.property) && + ALL_RETURNING_NODES.includes(node.parent.property.name); + + if (isLiteralNumber || hasForbiddenMethod) { + context.report({ + node: node, + messageId: 'noNodeAccess', + }); + } } - isMemberExpression(node.parent) && - isIdentifier(node.parent.property) && - ALL_RETURNING_NODES.includes(node.parent.property.name) && - context.report({ - node: node, - messageId: 'noNodeAccess', - }); } } From f63f2b0c2ae6e1812ceb5b2a6eda34550ba7d016 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 16:06:40 -0300 Subject: [PATCH 06/22] refactor(no-node-access): add scenario when no variable is declared --- lib/rules/no-node-access.ts | 51 +++++++++++++++-------- tests/lib/rules/no-node-access.test.ts | 57 +++++++++++++++++++++++++- 2 files changed, 90 insertions(+), 18 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index 09e8519a..e5b0a837 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -35,7 +35,23 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ create(context) { const variablesWithNodes: string[] = []; - function identifyVariablesWithNodes(node: TSESTree.MemberExpression) { + function showErrorForNodeAccess(node: TSESTree.MemberExpression) { + const isLiteralNumber = + isLiteral(node.property) && typeof node.property.value === 'number'; + const hasForbiddenMethod = + isIdentifier(node.property) && + ALL_RETURNING_NODES.includes(node.property.name); + + if (isLiteralNumber || hasForbiddenMethod) { + context.report({ + node: node, + loc: node.loc.start, + messageId: 'noNodeAccess', + }); + } + } + + function checkVariablesWithNodes(node: TSESTree.MemberExpression) { const callExpression = node.parent as TSESTree.CallExpression; const variableDeclarator = callExpression.parent as TSESTree.VariableDeclarator; const methodsNames = ALL_QUERIES_METHODS.filter( @@ -50,29 +66,30 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ } } - function showErrorForNodeAccess(node: TSESTree.Identifier) { + function checkDirectNodeAccess(node: TSESTree.Identifier) { if (variablesWithNodes.includes(node.name)) { if (isMemberExpression(node.parent)) { - const isLiteralNumber = - isLiteral(node.parent.property) && - typeof node.parent.property.value === 'number'; - const hasForbiddenMethod = - isIdentifier(node.parent.property) && - ALL_RETURNING_NODES.includes(node.parent.property.name); - - if (isLiteralNumber || hasForbiddenMethod) { - context.report({ - node: node, - messageId: 'noNodeAccess', - }); - } + showErrorForNodeAccess(node.parent); } } } + function checkDirectMethodCall(node: TSESTree.CallExpression) { + const methodsNames = ALL_QUERIES_METHODS.filter( + method => + isMemberExpression(node.callee) && + isIdentifier(node.callee.property) && + node.callee.property.name.includes(method) + ); + if (methodsNames.length && isMemberExpression(node.parent)) { + showErrorForNodeAccess(node.parent); + } + } + return { - ['VariableDeclarator > CallExpression > MemberExpression']: identifyVariablesWithNodes, - ['MemberExpression > Identifier']: showErrorForNodeAccess, + ['VariableDeclarator > CallExpression > MemberExpression']: checkVariablesWithNodes, + ['MemberExpression > Identifier']: checkDirectNodeAccess, + ['CallExpression']: checkDirectMethodCall, }; }, }); diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index 7f703a61..b5ca3f47 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -24,14 +24,34 @@ ruleTester.run(RULE_NAME, rule, { }, ], invalid: [ + { + code: `screen.getByText('submit').closest('button')`, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + expect(screen.getByText('submit').closest('button').textContent).toBe('Submit') + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, { code: ` const buttons = screen.getAllByRole('button'); const buttonA = buttons[1]; - expect(buttonA.lastChild).toBeInTheDocument(); `, errors: [ { + // error points to `buttons[1]` + line: 3, + column: 25, messageId: 'noNodeAccess', }, ], @@ -40,14 +60,41 @@ ruleTester.run(RULE_NAME, rule, { code: ` const buttons = screen.getAllByRole('button'); buttons[0].firstChild; + `, + errors: [ + { + // error points to `buttons[0]` + line: 3, + column: 9, + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + const buttons = screen.getAllByRole('button'); const buttonA = buttons[1]; expect(buttonA.lastChild).toBeInTheDocument(); `, errors: [ { + // error points to `buttons[1]` + line: 3, + column: 25, messageId: 'noNodeAccess', }, + ], + }, + { + code: ` + const buttons = screen.getAllByRole('button'); + expect(buttons[0]).toBeInTheDocument(); + `, + errors: [ { + // error points to `buttons[0]` + line: 3, + column: 16, messageId: 'noNodeAccess', }, ], @@ -59,6 +106,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { + // error points to `buttonText.closest` + line: 3, + column: 24, messageId: 'noNodeAccess', }, ], @@ -84,6 +134,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { + // error points to `buttons[1]` + line: 17, + column: 28, messageId: 'noNodeAccess', }, ], @@ -109,6 +162,8 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { + line: 17, + column: 33, messageId: 'noNodeAccess', }, ], From c1ecd66f0495b361857f839f00563943b950cde1 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 16:09:19 -0300 Subject: [PATCH 07/22] refactor(no-node-access): remove conditional --- lib/rules/no-node-access.ts | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index e5b0a837..a2c9c4c2 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -42,13 +42,12 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ isIdentifier(node.property) && ALL_RETURNING_NODES.includes(node.property.name); - if (isLiteralNumber || hasForbiddenMethod) { + (isLiteralNumber || hasForbiddenMethod) && context.report({ node: node, loc: node.loc.start, messageId: 'noNodeAccess', }); - } } function checkVariablesWithNodes(node: TSESTree.MemberExpression) { @@ -68,9 +67,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ function checkDirectNodeAccess(node: TSESTree.Identifier) { if (variablesWithNodes.includes(node.name)) { - if (isMemberExpression(node.parent)) { - showErrorForNodeAccess(node.parent); - } + isMemberExpression(node.parent) && showErrorForNodeAccess(node.parent); } } From 934bc1dbe625f2c3a52eb881a512290520f86841 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 16:45:19 -0300 Subject: [PATCH 08/22] refactor(utils): add DOM properties --- lib/utils.ts | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/utils.ts b/lib/utils.ts index b4f094fb..2737cda6 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -64,24 +64,32 @@ const ASYNC_UTILS = [ ]; const PROPERTIES_RETURNING_NODES = [ + 'activeElement', + 'children', 'firstChild', + 'firstElementChild', + 'fullscreenElement', 'lastChild', + 'lastElementChild', 'nextElementSibling', 'nextSibling', 'parentNode', + 'pointerLockElement', 'previousElementSibling', 'previousSibling', 'rootNode', + 'scripts', ]; const METHODS_RETURNING_NODES = [ 'closest', + 'getElementById', 'getElementsByClassName', + 'getElementsByName', 'getElementsByTagName', 'getElementsByTagNameNS', 'querySelector', 'querySelectorAll', - '' ]; export { From 70a366e831fc47cef850834daf5c14e3930c18ff Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 17:35:35 -0300 Subject: [PATCH 09/22] refactor(no-node-access): add scenario for accessing document directly --- lib/rules/no-node-access.ts | 11 ++++-- tests/lib/rules/no-node-access.test.ts | 46 +++++++++++++++++++++----- 2 files changed, 45 insertions(+), 12 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index a2c9c4c2..c3bf606f 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -14,12 +14,17 @@ const ALL_RETURNING_NODES = [ ...METHODS_RETURNING_NODES, ]; +const ALL_QUERIES_AND_RETURNING_NODES = [ + ...ALL_QUERIES_METHODS, + ...ALL_RETURNING_NODES, +]; + export default ESLintUtils.RuleCreator(getDocsUrl)({ name: RULE_NAME, meta: { type: 'problem', docs: { - description: 'Disallow the use of methods for direct Node access.', + description: 'Disallow direct Node access', category: 'Best Practices', recommended: 'error', }, @@ -53,7 +58,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ function checkVariablesWithNodes(node: TSESTree.MemberExpression) { const callExpression = node.parent as TSESTree.CallExpression; const variableDeclarator = callExpression.parent as TSESTree.VariableDeclarator; - const methodsNames = ALL_QUERIES_METHODS.filter( + const methodsNames = ALL_QUERIES_AND_RETURNING_NODES.filter( method => isIdentifier(node.property) && node.property.name.includes(method) ); @@ -72,7 +77,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ } function checkDirectMethodCall(node: TSESTree.CallExpression) { - const methodsNames = ALL_QUERIES_METHODS.filter( + const methodsNames = ALL_QUERIES_AND_RETURNING_NODES.filter( method => isMemberExpression(node.callee) && isIdentifier(node.callee.property) && diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index b5ca3f47..bc9c330b 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -15,17 +15,45 @@ ruleTester.run(RULE_NAME, rule, { `, }, { + code: ` + const buttonText = document.firstChild; + expect(buttonText.textContent).toBe('submit'); + `, + }, + { + // custom objects with the same properties names as DOM properties are allowed code: ` const myObj = { - firstChild: 'Some custom text' - } - const text = myObj.firstChild + method: () => ({ + firstChild: 'Some custom text' + }), + }; + const text = myObj.method().firstChild; `, }, ], invalid: [ { - code: `screen.getByText('submit').closest('button')`, + code: `document.getElementById('submit-btn').closest('button');`, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + const closestButton = document.getElementById('submit-btn').closest('button'); + expect(closestButton).toBeInTheDocument(); + `, + errors: [ + { + messageId: 'noNodeAccess', + }, + ], + }, + { + code: `screen.getByText('submit').closest('button');`, errors: [ { messageId: 'noNodeAccess', @@ -34,7 +62,7 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - expect(screen.getByText('submit').closest('button').textContent).toBe('Submit') + expect(screen.getByText('submit').closest('button').textContent).toBe('Submit'); `, errors: [ { @@ -127,10 +155,10 @@ ruleTester.run(RULE_NAME, rule, { \`; return container; - } + }; const exampleDOM = getExampleDOM(); const buttons = screen.getAllByRole(exampleDOM, 'button'); - const buttonText = buttons[1].firstChild + const buttonText = buttons[1].firstChild; `, errors: [ { @@ -155,10 +183,10 @@ ruleTester.run(RULE_NAME, rule, { \`; return container; - } + }; const exampleDOM = getExampleDOM(); const submitButton = screen.getByText(exampleDOM, 'Submit'); - const previousSibling = submitButton.previousSibling + const previousSibling = submitButton.previousSibling; `, errors: [ { From 291eddfa8971b2ad02c3ab7fda50faab34667501 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 17:36:10 -0300 Subject: [PATCH 10/22] docs(no-node-access): add readme --- docs/no-node-access.md | 44 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 docs/no-node-access.md diff --git a/docs/no-node-access.md b/docs/no-node-access.md new file mode 100644 index 00000000..9ad0de45 --- /dev/null +++ b/docs/no-node-access.md @@ -0,0 +1,44 @@ +# Disallow direct Node access (no-node-access) + +The Testing Library already provides methods for querying DOM elements. + +## Rule Details + +This rule aims to disallow DOM traversal using native HTML methods and properties, such as `closes`, `lastChild` and all that returns another Node element from an HTML tree. + +Examples of **incorrect** code for this rule: + +```js +screen.getByText('Submit').closest('button'); // chaining with Testing Library methods +``` + +```js +const buttons = screen.getAllByRole('button'); +const buttonA = buttons[1]; // getting the element directly from the array + +expect(buttonA.lastChild).toBeInTheDocument(); +``` + +```js +const buttonText = screen.getByText('Submit'); +const button = buttonText.closest('button'); +``` + +```js +document.getElementById('submit-btn').closest('button'); +``` + +Examples of **correct** code for this rule: + +```js +const buttonText = screen.getByRole('button'); +expect(buttonText.textContent).toBe('Submit'); +``` + +## Further Reading + +### Properties / methods that return another Node + +- [`Document`](https://developer.mozilla.org/en-US/docs/Web/API/Document) +- [`Element`](https://developer.mozilla.org/en-US/docs/Web/API/Element) +- [`Node`](https://developer.mozilla.org/en-US/docs/Web/API/Node) From e3328eee1abecbd33574835b0a71c333f7bac48c Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 17:38:00 -0300 Subject: [PATCH 11/22] refactor(utils): export const containing all properties and methods that return a Node --- lib/rules/no-node-access.ts | 12 +----------- lib/utils.ts | 6 ++++++ 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index c3bf606f..ae238b00 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -1,19 +1,9 @@ import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; import { isIdentifier, isMemberExpression, isLiteral } from '../node-utils'; -import { - getDocsUrl, - ALL_QUERIES_METHODS, - PROPERTIES_RETURNING_NODES, - METHODS_RETURNING_NODES, -} from '../utils'; +import { getDocsUrl, ALL_QUERIES_METHODS, ALL_RETURNING_NODES } from '../utils'; export const RULE_NAME = 'no-node-access'; -const ALL_RETURNING_NODES = [ - ...PROPERTIES_RETURNING_NODES, - ...METHODS_RETURNING_NODES, -]; - const ALL_QUERIES_AND_RETURNING_NODES = [ ...ALL_QUERIES_METHODS, ...ALL_RETURNING_NODES, diff --git a/lib/utils.ts b/lib/utils.ts index 2737cda6..c4da1d17 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -92,6 +92,11 @@ const METHODS_RETURNING_NODES = [ 'querySelectorAll', ]; +const ALL_RETURNING_NODES = [ + ...PROPERTIES_RETURNING_NODES, + ...METHODS_RETURNING_NODES, +]; + export { getDocsUrl, SYNC_QUERIES_VARIANTS, @@ -105,4 +110,5 @@ export { LIBRARY_MODULES, PROPERTIES_RETURNING_NODES, METHODS_RETURNING_NODES, + ALL_RETURNING_NODES, }; From 6cc564878c46c14fa0ff50746c6bdfb087b62003 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 18:39:22 -0300 Subject: [PATCH 12/22] docs(no-node-access): fix file location --- docs/{ => rules}/no-node-access.md | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename docs/{ => rules}/no-node-access.md (100%) diff --git a/docs/no-node-access.md b/docs/rules/no-node-access.md similarity index 100% rename from docs/no-node-access.md rename to docs/rules/no-node-access.md From af0ed98bf8aca31f4c37ea53ea0d87377c2c3e27 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 18:47:23 -0300 Subject: [PATCH 13/22] docs(readme): add no-node-access --- README.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/README.md b/README.md index 13ac18af..f32dace5 100644 --- a/README.md +++ b/README.md @@ -23,7 +23,9 @@ [![Tweet][tweet-badge]][tweet-url] + [![All Contributors](https://img.shields.io/badge/all_contributors-27-orange.svg?style=flat-square)](#contributors-) + ## Installation @@ -134,6 +136,7 @@ To enable this configuration use the `extends` property in your | [no-debug](docs/rules/no-debug.md) | Disallow the use of `debug` | ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [no-dom-import](docs/rules/no-dom-import.md) | Disallow importing from DOM Testing Library | ![angular-badge][] ![react-badge][] ![vue-badge][] | ![fixable-badge][] | | [no-manual-cleanup](docs/rules/no-manual-cleanup.md) | Disallow the use of `cleanup` | | | +| [no-node-access](docs/rules/no-node-access.md) | Disallow direct Node access | ![angular-badge][] ![react-badge][] ![vue-badge][] | | | | [no-promise-in-fire-event](docs/rules/no-promise-in-fire-event.md) | Disallow the use of promises passed to a `fireEvent` method | | | | [no-wait-for-empty-callback](docs/rules/no-wait-for-empty-callback.md) | Disallow empty callbacks for `waitFor` and `waitForElementToBeRemoved` | ![dom-badge][] ![angular-badge][] ![react-badge][] ![vue-badge][] | | | [prefer-explicit-assert](docs/rules/prefer-explicit-assert.md) | Suggest using explicit assertions rather than just `getBy*` queries | | | From 8f545d101a25d896f3c21fc0a6907b8660d48f90 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 19:12:54 -0300 Subject: [PATCH 14/22] refactor(no-node-access): change highlight location --- lib/rules/no-node-access.ts | 2 +- tests/lib/rules/no-node-access.test.ts | 28 ++++++++++++++------------ 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index ae238b00..382ea005 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -40,7 +40,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ (isLiteralNumber || hasForbiddenMethod) && context.report({ node: node, - loc: node.loc.start, + loc: node.property.loc.start, messageId: 'noNodeAccess', }); } diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index bc9c330b..e2bd734b 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -56,6 +56,8 @@ ruleTester.run(RULE_NAME, rule, { code: `screen.getByText('submit').closest('button');`, errors: [ { + line: 1, + column: 28, messageId: 'noNodeAccess', }, ], @@ -77,9 +79,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttons[1]` + // error points to `[1]` line: 3, - column: 25, + column: 33, messageId: 'noNodeAccess', }, ], @@ -91,9 +93,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttons[0]` + // error points to `[0]` line: 3, - column: 9, + column: 17, messageId: 'noNodeAccess', }, ], @@ -106,9 +108,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttons[1]` + // error points to `[1]` line: 3, - column: 25, + column: 33, messageId: 'noNodeAccess', }, ], @@ -120,9 +122,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttons[0]` + // error points to `[0]` line: 3, - column: 16, + column: 24, messageId: 'noNodeAccess', }, ], @@ -134,9 +136,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttonText.closest` + // error points to `closest` line: 3, - column: 24, + column: 35, messageId: 'noNodeAccess', }, ], @@ -162,9 +164,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `buttons[1]` + // error points to `[1]` line: 17, - column: 28, + column: 36, messageId: 'noNodeAccess', }, ], @@ -191,7 +193,7 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { line: 17, - column: 33, + column: 46, messageId: 'noNodeAccess', }, ], From 145a89bee59c34aae61059c307583c38209c2b2b Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 19:16:33 -0300 Subject: [PATCH 15/22] docs(no-node-access): fix typo --- docs/rules/no-node-access.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rules/no-node-access.md b/docs/rules/no-node-access.md index 9ad0de45..22c18767 100644 --- a/docs/rules/no-node-access.md +++ b/docs/rules/no-node-access.md @@ -4,7 +4,7 @@ The Testing Library already provides methods for querying DOM elements. ## Rule Details -This rule aims to disallow DOM traversal using native HTML methods and properties, such as `closes`, `lastChild` and all that returns another Node element from an HTML tree. +This rule aims to disallow DOM traversal using native HTML methods and properties, such as `closest`, `lastChild` and all that returns another Node element from an HTML tree. Examples of **incorrect** code for this rule: From bd13e954fe91104b38af32acc1bf54893f3f5e30 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Mon, 29 Jun 2020 19:23:12 -0300 Subject: [PATCH 16/22] refactor(utils): add missing property that returns a Node --- lib/utils.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/utils.ts b/lib/utils.ts index c4da1d17..7af670c8 100644 --- a/lib/utils.ts +++ b/lib/utils.ts @@ -73,6 +73,7 @@ const PROPERTIES_RETURNING_NODES = [ 'lastElementChild', 'nextElementSibling', 'nextSibling', + 'parentElement', 'parentNode', 'pointerLockElement', 'previousElementSibling', From f5eacca2c246a09f7c8483d3b6ab51535ce81855 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Fri, 3 Jul 2020 00:28:28 -0300 Subject: [PATCH 17/22] refactor(no-node-access): simplify checks triggering error for all methods with names matching the forbidden ones --- lib/rules/no-node-access.ts | 62 +++++-------------------------------- 1 file changed, 8 insertions(+), 54 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index 382ea005..2d17d930 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -1,14 +1,9 @@ import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; -import { isIdentifier, isMemberExpression, isLiteral } from '../node-utils'; -import { getDocsUrl, ALL_QUERIES_METHODS, ALL_RETURNING_NODES } from '../utils'; +import { getDocsUrl, ALL_RETURNING_NODES } from '../utils'; +import { isIdentifier } from '../node-utils'; export const RULE_NAME = 'no-node-access'; -const ALL_QUERIES_AND_RETURNING_NODES = [ - ...ALL_QUERIES_METHODS, - ...ALL_RETURNING_NODES, -]; - export default ESLintUtils.RuleCreator(getDocsUrl)({ name: RULE_NAME, meta: { @@ -28,60 +23,19 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ defaultOptions: [], create(context) { - const variablesWithNodes: string[] = []; - - function showErrorForNodeAccess(node: TSESTree.MemberExpression) { - const isLiteralNumber = - isLiteral(node.property) && typeof node.property.value === 'number'; - const hasForbiddenMethod = - isIdentifier(node.property) && - ALL_RETURNING_NODES.includes(node.property.name); - - (isLiteralNumber || hasForbiddenMethod) && + function showErrorForNodeAccess(node: TSESTree.Identifier) { + isIdentifier(node) && + ALL_RETURNING_NODES.includes(node.name) && context.report({ node: node, - loc: node.property.loc.start, + loc: node.loc.start, messageId: 'noNodeAccess', }); } - function checkVariablesWithNodes(node: TSESTree.MemberExpression) { - const callExpression = node.parent as TSESTree.CallExpression; - const variableDeclarator = callExpression.parent as TSESTree.VariableDeclarator; - const methodsNames = ALL_QUERIES_AND_RETURNING_NODES.filter( - method => - isIdentifier(node.property) && node.property.name.includes(method) - ); - - if (methodsNames.length) { - variablesWithNodes.push( - isIdentifier(variableDeclarator.id) && variableDeclarator.id.name - ); - } - } - - function checkDirectNodeAccess(node: TSESTree.Identifier) { - if (variablesWithNodes.includes(node.name)) { - isMemberExpression(node.parent) && showErrorForNodeAccess(node.parent); - } - } - - function checkDirectMethodCall(node: TSESTree.CallExpression) { - const methodsNames = ALL_QUERIES_AND_RETURNING_NODES.filter( - method => - isMemberExpression(node.callee) && - isIdentifier(node.callee.property) && - node.callee.property.name.includes(method) - ); - if (methodsNames.length && isMemberExpression(node.parent)) { - showErrorForNodeAccess(node.parent); - } - } - return { - ['VariableDeclarator > CallExpression > MemberExpression']: checkVariablesWithNodes, - ['MemberExpression > Identifier']: checkDirectNodeAccess, - ['CallExpression']: checkDirectMethodCall, + ['ExpressionStatement MemberExpression Identifier']: showErrorForNodeAccess, + ['VariableDeclarator Identifier']: showErrorForNodeAccess, }; }, }); From 9415c3be0ed980de25c3965ef2ad45baf0056983 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Fri, 3 Jul 2020 00:29:46 -0300 Subject: [PATCH 18/22] test(no-node-access): add more scenarios with destructuring --- tests/lib/rules/no-node-access.test.ts | 98 ++++++++++++++------------ 1 file changed, 53 insertions(+), 45 deletions(-) diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index e2bd734b..3a1d68d7 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -16,19 +16,9 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - const buttonText = document.firstChild; - expect(buttonText.textContent).toBe('submit'); - `, - }, - { - // custom objects with the same properties names as DOM properties are allowed - code: ` - const myObj = { - method: () => ({ - firstChild: 'Some custom text' - }), - }; - const text = myObj.method().firstChild; + const { getByText } = screen; + const button = getByRole('button'); + expect(button).toHaveTextContent('submit'); `, }, ], @@ -39,13 +29,13 @@ ruleTester.run(RULE_NAME, rule, { { messageId: 'noNodeAccess', }, + { + messageId: 'noNodeAccess', + }, ], }, { - code: ` - const closestButton = document.getElementById('submit-btn').closest('button'); - expect(closestButton).toBeInTheDocument(); - `, + code: `document.getElementById('submit-btn');`, errors: [ { messageId: 'noNodeAccess', @@ -56,6 +46,7 @@ ruleTester.run(RULE_NAME, rule, { code: `screen.getByText('submit').closest('button');`, errors: [ { + // error points to `closest` line: 1, column: 28, messageId: 'noNodeAccess', @@ -64,7 +55,7 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - expect(screen.getByText('submit').closest('button').textContent).toBe('Submit'); + expect(screen.getByText('submit').closest('button').textContent).toBe('Submit'); `, errors: [ { @@ -74,28 +65,31 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - const buttons = screen.getAllByRole('button'); - const buttonA = buttons[1]; + const { getByText } = render() + getByText('submit').closest('button'); `, errors: [ { - // error points to `[1]` - line: 3, - column: 33, messageId: 'noNodeAccess', }, ], }, { code: ` - const buttons = screen.getAllByRole('button'); - buttons[0].firstChild; + const closestButton = document.getElementById('submit-btn').closest('button'); + expect(closestButton).toBeInTheDocument(); `, errors: [ { - // error points to `[0]` - line: 3, - column: 17, + // error points to `getElementById` + line: 2, + column: 40, + messageId: 'noNodeAccess', + }, + { + // error points to `closest` + line: 2, + column: 69, messageId: 'noNodeAccess', }, ], @@ -103,42 +97,55 @@ ruleTester.run(RULE_NAME, rule, { { code: ` const buttons = screen.getAllByRole('button'); - const buttonA = buttons[1]; - expect(buttonA.lastChild).toBeInTheDocument(); + const childA = buttons[1].firstChild; + const button = buttons[2]; + button.lastChild `, errors: [ { - // error points to `[1]` + // error points to `firstChild` line: 3, - column: 33, + column: 35, + messageId: 'noNodeAccess', + }, + { + // error points to `lastChild` + line: 5, + column: 16, messageId: 'noNodeAccess', }, ], }, { code: ` - const buttons = screen.getAllByRole('button'); - expect(buttons[0]).toBeInTheDocument(); + const buttonText = screen.getByText('submit'); + const button = buttonText.closest('button'); `, errors: [ { - // error points to `[0]` - line: 3, - column: 24, messageId: 'noNodeAccess', }, ], }, { code: ` - const buttonText = screen.getByText('submit'); + const { getByText } = render() + const buttonText = getByText('submit'); const button = buttonText.closest('button'); `, errors: [ { - // error points to `closest` - line: 3, - column: 35, + messageId: 'noNodeAccess', + }, + ], + }, + { + code: ` + const { getByText } = render() + const button = getByText('submit').closest('button'); + `, + errors: [ + { messageId: 'noNodeAccess', }, ], @@ -164,9 +171,9 @@ ruleTester.run(RULE_NAME, rule, { `, errors: [ { - // error points to `[1]` + // error points to `firstChild` line: 17, - column: 36, + column: 39, messageId: 'noNodeAccess', }, ], @@ -188,12 +195,13 @@ ruleTester.run(RULE_NAME, rule, { }; const exampleDOM = getExampleDOM(); const submitButton = screen.getByText(exampleDOM, 'Submit'); - const previousSibling = submitButton.previousSibling; + const previousButton = submitButton.previousSibling; `, errors: [ { + // error points to `previousSibling` line: 17, - column: 46, + column: 45, messageId: 'noNodeAccess', }, ], From 0f1cd3645eb0c03140a88f5e472116b749ec7233 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Fri, 3 Jul 2020 00:30:18 -0300 Subject: [PATCH 19/22] docs(no-node-access): update examples --- docs/rules/no-node-access.md | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/docs/rules/no-node-access.md b/docs/rules/no-node-access.md index 22c18767..71b50851 100644 --- a/docs/rules/no-node-access.md +++ b/docs/rules/no-node-access.md @@ -14,9 +14,7 @@ screen.getByText('Submit').closest('button'); // chaining with Testing Library m ```js const buttons = screen.getAllByRole('button'); -const buttonA = buttons[1]; // getting the element directly from the array - -expect(buttonA.lastChild).toBeInTheDocument(); +expect(buttons[1].lastChild).toBeInTheDocument(); ``` ```js @@ -31,8 +29,8 @@ document.getElementById('submit-btn').closest('button'); Examples of **correct** code for this rule: ```js -const buttonText = screen.getByRole('button'); -expect(buttonText.textContent).toBe('Submit'); +const button = screen.getByRole('button'); +expect(button).toHaveTextContent('submit'); ``` ## Further Reading From 9d4f356eac2a34f23b43f66265db022cb8fd48ac Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Sun, 5 Jul 2020 11:20:30 -0300 Subject: [PATCH 20/22] refactor(no-node-access): narrow error cases --- lib/rules/no-node-access.ts | 12 ++++++------ tests/lib/rules/no-node-access.test.ts | 13 +++++++++++++ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index 2d17d930..79603eb0 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -23,19 +23,19 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ defaultOptions: [], create(context) { - function showErrorForNodeAccess(node: TSESTree.Identifier) { - isIdentifier(node) && - ALL_RETURNING_NODES.includes(node.name) && + function showErrorForNodeAccess(node: TSESTree.MemberExpression) { + isIdentifier(node.property) && + ALL_RETURNING_NODES.includes(node.property.name) && context.report({ node: node, - loc: node.loc.start, + loc: node.property.loc.start, messageId: 'noNodeAccess', }); } return { - ['ExpressionStatement MemberExpression Identifier']: showErrorForNodeAccess, - ['VariableDeclarator Identifier']: showErrorForNodeAccess, + ['ExpressionStatement MemberExpression']: showErrorForNodeAccess, + ['VariableDeclarator MemberExpression']: showErrorForNodeAccess, }; }, }); diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index 3a1d68d7..4c114b3c 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -14,6 +14,19 @@ ruleTester.run(RULE_NAME, rule, { const buttonText = screen.getByText('submit'); `, }, + { + code: ` + const { getByText } = screen + const firstChild = getByText('submit'); + expect(firstChild).toBeInTheDocument() + `, + }, + { + code: ` + const firstChild = screen.getByText('submit'); + expect(firstChild).toBeInTheDocument() + `, + }, { code: ` const { getByText } = screen; From ecb12fe1d820dd8048ce9847d0f48f9771174a84 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Fri, 10 Jul 2020 16:47:19 -0300 Subject: [PATCH 21/22] refactor(no-node-access): check imports to validate whether is importing a testing-library package | update examples and testing scenarios --- docs/rules/no-node-access.md | 26 +++++-- lib/rules/no-node-access.ts | 8 +++ tests/lib/rules/no-node-access.test.ts | 99 ++++++++++++++++++-------- 3 files changed, 99 insertions(+), 34 deletions(-) diff --git a/docs/rules/no-node-access.md b/docs/rules/no-node-access.md index 71b50851..78d775dd 100644 --- a/docs/rules/no-node-access.md +++ b/docs/rules/no-node-access.md @@ -9,30 +9,48 @@ This rule aims to disallow DOM traversal using native HTML methods and propertie Examples of **incorrect** code for this rule: ```js +import { screen } from '@testing-library/react'; + screen.getByText('Submit').closest('button'); // chaining with Testing Library methods ``` ```js +import { screen } from '@testing-library/react'; + const buttons = screen.getAllByRole('button'); expect(buttons[1].lastChild).toBeInTheDocument(); ``` ```js +import { screen } from '@testing-library/react'; + const buttonText = screen.getByText('Submit'); const button = buttonText.closest('button'); ``` -```js -document.getElementById('submit-btn').closest('button'); -``` - Examples of **correct** code for this rule: ```js +import { screen } from '@testing-library/react'; + const button = screen.getByRole('button'); expect(button).toHaveTextContent('submit'); ``` +```js +import { render, within } from '@testing-library/react'; + +const { getByLabelText } = render(); +const signinModal = getByLabelText('Sign In'); +within(signinModal).getByPlaceholderText('Username'); +``` + +```js +// If is not importing a testing-library package + +document.getElementById('submit-btn').closest('button'); +``` + ## Further Reading ### Properties / methods that return another Node diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index 79603eb0..8c70dd75 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -23,9 +23,16 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ defaultOptions: [], create(context) { + let isTestingEnv = false; + + function checkTestingEnvironment(node: TSESTree.ImportDeclaration) { + isTestingEnv = /testing-library/g.test(node.source.value as string); + } + function showErrorForNodeAccess(node: TSESTree.MemberExpression) { isIdentifier(node.property) && ALL_RETURNING_NODES.includes(node.property.name) && + isTestingEnv && context.report({ node: node, loc: node.property.loc.start, @@ -34,6 +41,7 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ } return { + ['ImportDeclaration']: checkTestingEnvironment, ['ExpressionStatement MemberExpression']: showErrorForNodeAccess, ['VariableDeclarator MemberExpression']: showErrorForNodeAccess, }; diff --git a/tests/lib/rules/no-node-access.test.ts b/tests/lib/rules/no-node-access.test.ts index 4c114b3c..ee432767 100644 --- a/tests/lib/rules/no-node-access.test.ts +++ b/tests/lib/rules/no-node-access.test.ts @@ -11,11 +11,15 @@ ruleTester.run(RULE_NAME, rule, { valid: [ { code: ` + import { screen } from '@testing-library/react'; + const buttonText = screen.getByText('submit'); `, }, { code: ` + import { screen } from '@testing-library/react'; + const { getByText } = screen const firstChild = getByText('submit'); expect(firstChild).toBeInTheDocument() @@ -23,21 +27,52 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { screen } from '@testing-library/react'; + const firstChild = screen.getByText('submit'); expect(firstChild).toBeInTheDocument() `, }, { code: ` + import { screen } from '@testing-library/react'; + const { getByText } = screen; const button = getByRole('button'); expect(button).toHaveTextContent('submit'); `, }, + { + code: ` + import { render, within } from '@testing-library/react'; + + const { getByLabelText } = render(); + const signinModal = getByLabelText('Sign In'); + within(signinModal).getByPlaceholderText('Username'); + `, + }, + { + code: ` + const Component = props => { + return
{props.children}
+ } + `, + }, + { + // Not importing a testing-library package + code: ` + const closestButton = document.getElementById('submit-btn').closest('button'); + expect(closestButton).toBeInTheDocument(); + `, + }, ], invalid: [ { - code: `document.getElementById('submit-btn').closest('button');`, + code: ` + import { screen } from '@testing-library/react'; + + const button = document.getElementById('submit-btn').closest('button'); + `, errors: [ { messageId: 'noNodeAccess', @@ -48,7 +83,11 @@ ruleTester.run(RULE_NAME, rule, { ], }, { - code: `document.getElementById('submit-btn');`, + code: ` + import { screen } from '@testing-library/react'; + + document.getElementById('submit-btn'); + `, errors: [ { messageId: 'noNodeAccess', @@ -56,19 +95,25 @@ ruleTester.run(RULE_NAME, rule, { ], }, { - code: `screen.getByText('submit').closest('button');`, + code: ` + import { screen } from '@testing-library/react'; + + screen.getByText('submit').closest('button'); + `, errors: [ { // error points to `closest` - line: 1, - column: 28, + line: 4, + column: 36, messageId: 'noNodeAccess', }, ], }, { code: ` - expect(screen.getByText('submit').closest('button').textContent).toBe('Submit'); + import { screen } from '@testing-library/react'; + + expect(screen.getByText('submit').closest('button').textContent).toBe('Submit'); `, errors: [ { @@ -78,6 +123,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { render } from '@testing-library/react'; + const { getByText } = render() getByText('submit').closest('button'); `, @@ -89,26 +136,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` - const closestButton = document.getElementById('submit-btn').closest('button'); - expect(closestButton).toBeInTheDocument(); - `, - errors: [ - { - // error points to `getElementById` - line: 2, - column: 40, - messageId: 'noNodeAccess', - }, - { - // error points to `closest` - line: 2, - column: 69, - messageId: 'noNodeAccess', - }, - ], - }, - { - code: ` + import { screen } from '@testing-library/react'; + const buttons = screen.getAllByRole('button'); const childA = buttons[1].firstChild; const button = buttons[2]; @@ -117,13 +146,13 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { // error points to `firstChild` - line: 3, + line: 5, column: 35, messageId: 'noNodeAccess', }, { // error points to `lastChild` - line: 5, + line: 7, column: 16, messageId: 'noNodeAccess', }, @@ -131,6 +160,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { screen } from '@testing-library/react'; + const buttonText = screen.getByText('submit'); const button = buttonText.closest('button'); `, @@ -142,6 +173,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { render } from '@testing-library/react'; + const { getByText } = render() const buttonText = getByText('submit'); const button = buttonText.closest('button'); @@ -154,6 +187,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { render } from '@testing-library/react'; + const { getByText } = render() const button = getByText('submit').closest('button'); `, @@ -165,6 +200,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { screen } from '@testing-library/react'; + function getExampleDOM() { const container = document.createElement('div'); container.innerHTML = \` @@ -185,7 +222,7 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { // error points to `firstChild` - line: 17, + line: 19, column: 39, messageId: 'noNodeAccess', }, @@ -193,6 +230,8 @@ ruleTester.run(RULE_NAME, rule, { }, { code: ` + import { screen } from '@testing-library/react'; + function getExampleDOM() { const container = document.createElement('div'); container.innerHTML = \` @@ -213,7 +252,7 @@ ruleTester.run(RULE_NAME, rule, { errors: [ { // error points to `previousSibling` - line: 17, + line: 19, column: 45, messageId: 'noNodeAccess', }, From eb8d4a36653c819b27d7016c92d467353b2af109 Mon Sep 17 00:00:00 2001 From: thebinaryfelix Date: Fri, 10 Jul 2020 16:52:35 -0300 Subject: [PATCH 22/22] refactor(no-node-access): rename variable --- lib/rules/no-node-access.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/rules/no-node-access.ts b/lib/rules/no-node-access.ts index 8c70dd75..e0a12933 100644 --- a/lib/rules/no-node-access.ts +++ b/lib/rules/no-node-access.ts @@ -23,16 +23,16 @@ export default ESLintUtils.RuleCreator(getDocsUrl)({ defaultOptions: [], create(context) { - let isTestingEnv = false; + let isImportingTestingLibrary = false; function checkTestingEnvironment(node: TSESTree.ImportDeclaration) { - isTestingEnv = /testing-library/g.test(node.source.value as string); + isImportingTestingLibrary = /testing-library/g.test(node.source.value as string); } function showErrorForNodeAccess(node: TSESTree.MemberExpression) { isIdentifier(node.property) && ALL_RETURNING_NODES.includes(node.property.name) && - isTestingEnv && + isImportingTestingLibrary && context.report({ node: node, loc: node.property.loc.start,