Skip to content
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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Oct 17, 2024
Merged

boolean-conditional-rendering rule #121

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
dcfdb61
boolean-conditional-rendering rule
rayane-djouah Oct 3, 2024
e12b697
Merge branch 'Expensify:main' into add-eslint-rule-for-validating-lef…
rayane-djouah Oct 3, 2024
4ba4cbc
Fix lint errors
rayane-djouah Oct 3, 2024
46040c1
Use TypeFlags from TypeScript API instead of hardcoded values
rayane-djouah Oct 3, 2024
c3505c1
remove line break
rayane-djouah Oct 3, 2024
fbbe2b2
add a test case for boolean | undefined
rayane-djouah Oct 9, 2024
1b61ea9
Merge branch 'Expensify:main' into add-eslint-rule-for-validating-lef…
rayane-djouah Oct 9, 2024
48f83ec
Fix lint errors
rayane-djouah Oct 9, 2024
9f1b5bc
Merge branch 'main' into add-eslint-rule-for-validating-left-side-con…
rayane-djouah Oct 16, 2024
7ae1666
remove a test case
rayane-djouah Oct 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
66 changes: 66 additions & 0 deletions eslint-plugin-expensify/boolean-conditional-rendering.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,66 @@
/* 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}}".',
rayane-djouah marked this conversation as resolved.
Show resolved Hide resolved
},
},
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,
))
);
}
Comment on lines +24 to +41
Copy link

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 😕

Copy link
Contributor Author

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?

Copy link
Contributor Author

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?

Copy link
Contributor

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 !!

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),
},
});
}
},
};
},
};
260 changes: 260 additions & 0 deletions eslint-plugin-expensify/tests/boolean-conditional-rendering.test.js
View file
Open in desktop
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, {
Copy link
Contributor

Choose a reason for hiding this comment

The 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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added a test case to handle boolean | undefined in this commit: fbbe2b2. However, the test is failing, as seen in the workflow here: GitHub Actions Run. @szymonrybczak @mountiny Any insights on why this might be happening?

Copy link
Contributor

Choose a reason for hiding this comment

The 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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But should we raise an error if the value is boolean | undefined? IMO we shouldn't, if the value is equal undefined the condition will be false.

Copy link
Contributor

Choose a reason for hiding this comment

The 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'},
},
],
},
],
});
Empty file added eslint-plugin-expensify/tests/react.tsx
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Empty file

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is needed to fix this tests error:

Screenshot 2024-10-03 at 11 59 12 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is similar to https://github.com/Expensify/eslint-config-expensify/blob/main/eslint-plugin-expensify/tests/file.ts

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://typescript-eslint.io/packages/rule-tester/#type-aware-testing

Screenshot 2024-10-04 at 12 09 19 AM

Empty file.