-
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
boolean-conditional-rendering rule #121
Conversation
Draft PR for testing: Expensify/App#50190 |
@szymonrybczak @mountiny PR ready for review |
Eslint run on |
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 you please review?
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.
Empty file
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.
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.
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.
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, | ||
)) | ||
); | ||
} |
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 !!
}, | ||
}); | ||
|
||
ruleTester.run('boolean-conditional-rendering', rule, { |
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.
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 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?
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.
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 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.
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 better to be more strict when it comes to these than less, it should be easily resolvable by using !!
@rayane-djouah @szymonrybczak how is this look ing, are you able to debug the jest tests? |
Hm, this issue is very strange. For some reason when checking flags in an union type |
Yeah, it's odd. Shouldn't the flags for the |
Lets skip this and allow boolean || undefined since the effect will be the same. Lets not spend more time on this if we have not figured this out by now |
Please pull |
Updated |
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.
Thanks!
🚀 Published in 2.0.71 🎉 |
Expensify/App#50154
Note: you need to add
'rulesdir/boolean-conditional-rendering': 'error',
here.Draft PR for testing: Expensify/App#50190