-
Notifications
You must be signed in to change notification settings - Fork 150
feat: add render-result-naming-convention rule #200
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
Merged
Merged
Changes from 16 commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
d8c5ee1
feat: rule skeleton
Belco90 fe4be39
test: first round
Belco90 1ed0def
feat: rule implementation round 1
Belco90 ce61d56
refactor: move hasTestingLibraryImportModule
Belco90 3e54074
test: fix invalid lines
Belco90 d55ed50
feat: check imported module
Belco90 c56b7dc
feat: check imported render renamed
Belco90 4837c3d
feat: check custom render
Belco90 a2ec0a2
test: add more valid tests for custom render functions
Belco90 0ac133c
test: update tests for render wrapper functions
Belco90 a205e4d
docs: add rule docs
Belco90 f452410
test: increase coverage up to 100%
Belco90 c326afb
fix: add rule meta description
Belco90 2fde16d
docs: update rule details to mention screen object
Belco90 112d565
refactor: return as soon as conditions are not met
Belco90 ce7a207
feat: check wildcard imports
Belco90 92f769e
refactor: rename default import
Belco90 294645f
docs: include render result link
Belco90 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,77 @@ | ||
| # Enforce a valid naming for return value from `render` (render-result-naming-convention) | ||
|
|
||
| > The name `wrapper` is old cruft from `enzyme` and we don't need that here. The return value from `render` is not "wrapping" anything. It's simply a collection of utilities that you should actually not often need anyway. | ||
|
|
||
| ## Rule Details | ||
|
|
||
| This rule aims to ensure the return value from `render` is named properly. | ||
|
|
||
| Ideally, you should destructure the minimum utils that you need from `render`, combined with using queries from [`screen` object](https://github.com/testing-library/eslint-plugin-testing-library/blob/master/docs/rules/prefer-screen-queries.md). In case you need to save the collection of utils returned in a variable, its name should be either `view` or `utils`, as `render` is not wrapping anything: it's just returning a collection of utilities. Every other name for that variable will be considered invalid. | ||
|
|
||
| To sum up these rules, the allowed naming convention for return value from `render` is: | ||
|
|
||
| - destructuring | ||
| - `view` | ||
| - `utils` | ||
|
|
||
| Examples of **incorrect** code for this rule: | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // return value from `render` shouldn't be kept in a var called "wrapper" | ||
| const wrapper = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // return value from `render` shouldn't be kept in a var called "component" | ||
| const component = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // to sum up: return value from `render` shouldn't be kept in a var called other than "view" or "utils" | ||
| const somethingElse = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| Examples of **correct** code for this rule: | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // destructuring return value from `render` is correct | ||
| const { unmount, rerender } = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // keeping return value from `render` in a var called "view" is correct | ||
| const view = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| ```javascript | ||
| import { render } from '@testing-library/framework'; | ||
|
|
||
| // ... | ||
|
|
||
| // keeping return value from `render` in a var called "utils" is correct | ||
| const utils = render(<SomeComponent />); | ||
| ``` | ||
|
|
||
| ## Further Reading | ||
|
|
||
| - [Common Mistakes with React Testing Library](https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#using-wrapper-as-the-variable-name-for-the-return-value-from-render) | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,145 @@ | ||
| import { ESLintUtils, TSESTree } from '@typescript-eslint/experimental-utils'; | ||
| import { getDocsUrl, hasTestingLibraryImportModule } from '../utils'; | ||
| import { | ||
| isCallExpression, | ||
| isIdentifier, | ||
| isImportSpecifier, | ||
| isMemberExpression, | ||
| isObjectPattern, | ||
| isRenderVariableDeclarator, | ||
| } from '../node-utils'; | ||
|
|
||
| export const RULE_NAME = 'render-result-naming-convention'; | ||
|
|
||
| const ALLOWED_VAR_NAMES = ['view', 'utils']; | ||
| const ALLOWED_VAR_NAMES_TEXT = ALLOWED_VAR_NAMES.map( | ||
| name => '`' + name + '`' | ||
| ).join(', '); | ||
|
|
||
| export default ESLintUtils.RuleCreator(getDocsUrl)({ | ||
| name: RULE_NAME, | ||
| meta: { | ||
| type: 'suggestion', | ||
| docs: { | ||
| description: 'Enforce a valid naming for return value from `render`', | ||
| category: 'Best Practices', | ||
| recommended: false, | ||
| }, | ||
| messages: { | ||
| invalidRenderResultName: `\`{{ varName }}\` is not a recommended name for \`render\` returned value. Instead, you should destructure it, or call it using one of the valid choices: ${ALLOWED_VAR_NAMES_TEXT}`, | ||
| }, | ||
| fixable: null, | ||
| schema: [ | ||
| { | ||
| type: 'object', | ||
| properties: { | ||
| renderFunctions: { | ||
| type: 'array', | ||
| }, | ||
| }, | ||
| }, | ||
| ], | ||
| }, | ||
| defaultOptions: [ | ||
| { | ||
| renderFunctions: [], | ||
| }, | ||
| ], | ||
|
|
||
| create(context, [options]) { | ||
| const { renderFunctions } = options; | ||
| let renderAlias: string | undefined; | ||
| let wildcardImportName: string | undefined; | ||
|
|
||
| return { | ||
| // check named imports | ||
| ImportDeclaration(node: TSESTree.ImportDeclaration) { | ||
Belco90 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (!hasTestingLibraryImportModule(node)) { | ||
| return; | ||
| } | ||
| const renderImport = node.specifiers.find( | ||
| node => isImportSpecifier(node) && node.imported.name === 'render' | ||
| ); | ||
|
|
||
| if (!renderImport) { | ||
| return; | ||
| } | ||
|
|
||
| renderAlias = renderImport.local.name; | ||
| }, | ||
| // check wildcard imports | ||
| 'ImportDeclaration ImportNamespaceSpecifier'( | ||
| node: TSESTree.ImportNamespaceSpecifier | ||
| ) { | ||
| if ( | ||
| !hasTestingLibraryImportModule( | ||
| node.parent as TSESTree.ImportDeclaration | ||
| ) | ||
| ) { | ||
| return; | ||
| } | ||
|
|
||
| wildcardImportName = node.local.name; | ||
| }, | ||
| VariableDeclarator(node: TSESTree.VariableDeclarator) { | ||
| // check if destructuring return value from render | ||
| if (isObjectPattern(node.id)) { | ||
| return; | ||
| } | ||
|
|
||
| const isValidRenderDeclarator = isRenderVariableDeclarator(node, [ | ||
| ...renderFunctions, | ||
| renderAlias, | ||
| ]); | ||
| const isValidWildcardImport = !!wildcardImportName; | ||
|
|
||
| // check if is a Testing Library related import | ||
| if (!isValidRenderDeclarator && !isValidWildcardImport) { | ||
| return; | ||
| } | ||
|
|
||
| const renderFunctionName = | ||
| isCallExpression(node.init) && | ||
| isIdentifier(node.init.callee) && | ||
| node.init.callee.name; | ||
|
|
||
| const renderFunctionObjectName = | ||
| isCallExpression(node.init) && | ||
| isMemberExpression(node.init.callee) && | ||
| isIdentifier(node.init.callee.property) && | ||
| isIdentifier(node.init.callee.object) && | ||
| node.init.callee.property.name === 'render' && | ||
| node.init.callee.object.name; | ||
|
|
||
| const isRenderAlias = !!renderAlias; | ||
| const isCustomRender = renderFunctions.includes(renderFunctionName); | ||
| const isWildCardRender = | ||
| renderFunctionObjectName && | ||
| renderFunctionObjectName === wildcardImportName; | ||
|
|
||
| // check if is a qualified render function | ||
| if (!isRenderAlias && !isCustomRender && !isWildCardRender) { | ||
| return; | ||
| } | ||
|
|
||
| const renderResultName = isIdentifier(node.id) && node.id.name; | ||
| const isAllowedRenderResultName = ALLOWED_VAR_NAMES.includes( | ||
| renderResultName | ||
| ); | ||
|
|
||
| // check if return value var name is allowed | ||
| if (isAllowedRenderResultName) { | ||
| return; | ||
| } | ||
|
|
||
| context.report({ | ||
| node, | ||
| messageId: 'invalidRenderResultName', | ||
| data: { | ||
| varName: renderResultName, | ||
| }, | ||
| }); | ||
| }, | ||
| }; | ||
| }, | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.