-
Notifications
You must be signed in to change notification settings - Fork 27
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
boolean-conditional-rendering rule #121
Changes from all commits
dcfdb61
e12b697
4ba4cbc
46040c1
c3505c1
fbbe2b2
1b61ea9
48f83ec
9f1b5bc
7ae1666
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,65 @@ | ||
/* eslint-disable no-bitwise */ | ||
const _ = require('underscore'); | ||
const {ESLintUtils} = require('@typescript-eslint/utils'); | ||
const ts = require('typescript'); | ||
|
||
module.exports = { | ||
name: 'boolean-conditional-rendering', | ||
meta: { | ||
type: 'problem', | ||
docs: { | ||
description: 'Enforce boolean conditions in React conditional rendering', | ||
recommended: 'error', | ||
}, | ||
schema: [], | ||
messages: { | ||
nonBooleanConditional: 'The left side of conditional rendering should be a boolean, not "{{type}}".', | ||
}, | ||
}, | ||
defaultOptions: [], | ||
create(context) { | ||
function isJSXElement(node) { | ||
return node.type === 'JSXElement' || node.type === 'JSXFragment'; | ||
} | ||
function isBoolean(type) { | ||
return ( | ||
(type.getFlags() | ||
& (ts.TypeFlags.Boolean | ||
| ts.TypeFlags.BooleanLike | ||
| ts.TypeFlags.BooleanLiteral)) | ||
!== 0 | ||
|| (type.isUnion() | ||
&& _.every( | ||
type.types, | ||
t => (t.getFlags() | ||
& (ts.TypeFlags.Boolean | ||
| ts.TypeFlags.BooleanLike | ||
| ts.TypeFlags.BooleanLiteral)) | ||
!== 0, | ||
)) | ||
); | ||
} | ||
const parserServices = ESLintUtils.getParserServices(context); | ||
const typeChecker = parserServices.program.getTypeChecker(); | ||
return { | ||
LogicalExpression(node) { | ||
if (!(node.operator === '&&' && isJSXElement(node.right))) { | ||
return; | ||
} | ||
const leftType = typeChecker.getTypeAtLocation( | ||
parserServices.esTreeNodeToTSNodeMap.get(node.left), | ||
); | ||
if (!isBoolean(leftType)) { | ||
const baseType = typeChecker.getBaseTypeOfLiteralType(leftType); | ||
context.report({ | ||
node: node.left, | ||
messageId: 'nonBooleanConditional', | ||
data: { | ||
type: typeChecker.typeToString(baseType), | ||
}, | ||
}); | ||
} | ||
}, | ||
}; | ||
}, | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,260 @@ | ||
const RuleTester = require('@typescript-eslint/rule-tester').RuleTester; | ||
const rule = require('../boolean-conditional-rendering'); | ||
|
||
const ruleTester = new RuleTester({ | ||
parser: '@typescript-eslint/parser', | ||
parserOptions: { | ||
project: './tsconfig.json', | ||
tsconfigRootDir: __dirname, | ||
sourceType: 'module', | ||
ecmaVersion: 2020, | ||
ecmaFeatures: { | ||
jsx: true, | ||
}, | ||
}, | ||
}); | ||
|
||
ruleTester.run('boolean-conditional-rendering', rule, { | ||
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. Can you please add a test for the boolean | undefined case we have seen a lot in the app? 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've added a test case to handle 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 test seems right to me and considering this is correctly flagging in App based on your testing, I am not sure. Asked Szymon in Slack 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. But should we raise an error if the value is 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 think its better to be more strict when it comes to these than less, it should be easily resolvable by using !! |
||
valid: [ | ||
{ | ||
code: ` | ||
const isActive = true; | ||
isActive && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const isActive = false; | ||
isActive && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const isVisible = Boolean(someValue); | ||
isVisible && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const user = { isLoggedIn: true, isBlocked: false }; | ||
const isAuthorized = user.isLoggedIn && !user.isBlocked; | ||
isAuthorized && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
function isAuthenticated() { return true; } | ||
isAuthenticated() && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const isReady: boolean = true; | ||
isReady && <ReadyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const isNotActive = !isActive; | ||
isNotActive && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const condition = !!someValue; | ||
condition && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const condition = someValue as boolean; | ||
condition && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
enum Status { Active, Inactive } | ||
const isActive = status === Status.Active; | ||
isActive && <MyComponent />; | ||
`, | ||
}, | ||
{ | ||
code: ` | ||
const isAvailable = checkAvailability(); | ||
isAvailable && <MyComponent />; | ||
function checkAvailability(): boolean { return true; } | ||
`, | ||
}, | ||
], | ||
invalid: [ | ||
{ | ||
code: ` | ||
const condition = "string"; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'string'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = 42; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'number'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = []; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'any[]'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = {}; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: '{}'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = null; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'any'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = undefined; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'any'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = () => {}; | ||
condition() && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'void'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition: unknown = someValue; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'unknown'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition: boolean | string = someValue; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'string | boolean'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = someObject?.property; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'any'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
enum Status { Active, Inactive } | ||
const status = Status.Active; | ||
status && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'string'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = Promise.resolve(true); | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'Promise<boolean>'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
function getValue() { return "value"; } | ||
getValue() && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'string'}, | ||
}, | ||
], | ||
}, | ||
{ | ||
code: ` | ||
const condition = someValue as string; | ||
condition && <MyComponent />; | ||
`, | ||
errors: [ | ||
{ | ||
messageId: 'nonBooleanConditional', | ||
data: {type: 'string'}, | ||
}, | ||
], | ||
}, | ||
], | ||
}); |
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.
I see that mainly this part changed from my initial PoC, and it seems to be a lot better as it uses TypeScript helper functions and is more robust and accurate. Because of this change it seems like there are a lot more errors in the Expensify codebase because this condition is actually correctly validating all possible cases and initial 1 error turns out to be 150+ errors. 🥲
However I did take a look at some of the errors and around half of them are because the left-side value is of type
boolean | undefined
I believe we shouldn't raise an error for such case, rest of them are accurate.In such a scenario I don't should we proceed with implementing this rule, as there'll be a lot of changed in Expensify codebase 😕
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.
@szymonrybczak Can we resolve most errors by using
!!
to explicitly convert values to booleans?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.
@mountiny What do you think of the above comments?
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.
I think its ok to make 150 changes if it will lead to cleaner code and if we can just fix it with !!