-
Notifications
You must be signed in to change notification settings - Fork 150
feat: add no-node-access rule #190
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
Changes from all commits
b0a1157
9ebf44e
4d41edc
9ebf3d2
cba1f13
5092889
f63f2b0
c1ecd66
934bc1d
70a366e
291eddf
e3328ee
6cc5648
af0ed98
8f545d1
145a89b
bd13e95
f5eacca
9415c3b
0f1cd36
ba40a64
9d4f356
ecb12fe
eb8d4a3
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 |
---|---|---|
@@ -0,0 +1,60 @@ | ||
# 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 `closest`, `lastChild` and all that returns another Node element from an HTML tree. | ||
|
||
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'); | ||
``` | ||
|
||
Examples of **correct** code for this rule: | ||
|
||
```js | ||
import { screen } from '@testing-library/react'; | ||
|
||
const button = screen.getByRole('button'); | ||
expect(button).toHaveTextContent('submit'); | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
``` | ||
|
||
```js | ||
import { render, within } from '@testing-library/react'; | ||
|
||
const { getByLabelText } = render(<MyComponent />); | ||
const signinModal = getByLabelText('Sign In'); | ||
within(signinModal).getByPlaceholderText('Username'); | ||
``` | ||
|
||
```js | ||
// If is not importing a testing-library package | ||
|
||
document.getElementById('submit-btn').closest('button'); | ||
``` | ||
thebinaryfelix marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
## 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) |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,49 @@ | ||
import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
import { getDocsUrl, ALL_RETURNING_NODES } from '../utils'; | ||
import { isIdentifier } from '../node-utils'; | ||
|
||
export const RULE_NAME = 'no-node-access'; | ||
|
||
export default ESLintUtils.RuleCreator(getDocsUrl)({ | ||
name: RULE_NAME, | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
description: 'Disallow direct Node access', | ||
category: 'Best Practices', | ||
recommended: 'error', | ||
}, | ||
messages: { | ||
noNodeAccess: | ||
'Avoid direct Node access. Prefer using the methods from Testing Library.', | ||
}, | ||
fixable: null, | ||
schema: [], | ||
}, | ||
defaultOptions: [], | ||
|
||
create(context) { | ||
let isImportingTestingLibrary = false; | ||
|
||
function checkTestingEnvironment(node: TSESTree.ImportDeclaration) { | ||
isImportingTestingLibrary = /testing-library/g.test(node.source.value as string); | ||
} | ||
|
||
function showErrorForNodeAccess(node: TSESTree.MemberExpression) { | ||
isIdentifier(node.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. Nit, move 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. Oops sorry! We approved at the same time and I merged it without realizing about this. |
||
ALL_RETURNING_NODES.includes(node.property.name) && | ||
isImportingTestingLibrary && | ||
context.report({ | ||
node: node, | ||
loc: node.property.loc.start, | ||
messageId: 'noNodeAccess', | ||
}); | ||
} | ||
|
||
return { | ||
['ImportDeclaration']: checkTestingEnvironment, | ||
['ExpressionStatement MemberExpression']: showErrorForNodeAccess, | ||
['VariableDeclarator MemberExpression']: showErrorForNodeAccess, | ||
}; | ||
}, | ||
}); |
Uh oh!
There was an error while loading. Please reload this page.