chore: enable more ESLint rules and fix existing violations #47
Conversation
src/App.tsx
Outdated
| @@ -27,13 +27,13 @@ function App() { | |||
|
|
|||
| return ( | |||
| <PanelGroup autoSaveId="refstudio" direction="horizontal"> | |||
| <Panel defaultSize={20} collapsible className="overflow-scroll p-4"> | |||
| <Panel className="overflow-scroll p-4" collapsible defaultSize={20}> | |||
There was a problem hiding this comment.
This change comes from react/jsx-sort-props.
There was a problem hiding this comment.
If this is auto-sortable on save it's ok for me.
| @@ -1,16 +1,16 @@ | |||
| .collapsible-block { | |||
| /* @apply debug; */ | |||
| @apply flex my-2 p-2 pl-0 rounded-lg; | |||
| @apply my-2 flex rounded-lg p-2 pl-0; | |||
There was a problem hiding this comment.
There's some diff noise here from running yarn fmt
There was a problem hiding this comment.
Somehow the auto-sort for did not pick up css files.
| 'Mod-Enter': () => { | ||
| return this.editor | ||
| 'Mod-Enter': () => | ||
| this.editor |
There was a problem hiding this comment.
This is the arrow-body-style rule.
There was a problem hiding this comment.
Don't you prefer block body for arrow functions when it has a line break? I find it easier to read, and I always think about a well-known security issue a few years ago because an if/else didn't have curly braces and someone added another instruction (so, another line) which would run every time (regardless of the condition)
There was a problem hiding this comment.
I don't mind this, actually! The if/else continuation line issue is very real, which is why I think curly is a good idea. But since the thing on the RHS of => is an expression, not a statement, I think there's less room for trouble. In most cases if you added an extra statement after an arrow function with a line break, it would be a syntax error or create dead code (which TypeScript would complain about).
| }, [editorContent, onSelectionChange]); | ||
|
|
||
| useEffect(() => { | ||
| if (!editor) return; | ||
| if (!editor) { | ||
| return; |
There was a problem hiding this comment.
This is the curly rule in action.
There was a problem hiding this comment.
I do like to this type of "fast-return" conditions being a single line so that we can read the "pre-conditions" at the top without any line breaks.
Sometimes you have 2 or 3 (unrelated) conditions you need to check (ex: with some ! expressions). IMO it's a good practice to keep them different statements (ex: for diff and legibility).
With this rule we would have 3 rules x 3 lines = 9 lines of code instead of only 3 like the example below.
useEffect(() => {
if (!editor) {
return
}
if (draft) {
return
}
if(anotherRule & anotherRule.field) {
return
}
// ...
}, [...])vs
useEffect(() => {
if (!editor) return
if (draft) return
if(anotherRule & anotherRule.field) return
// ...
}, [...])There was a problem hiding this comment.
If you guys would like to set curly: multi-line (https://eslint.org/docs/latest/rules/curly#multi-line) that's fine by me. My preference would be slightly more strict than that rule, though. If you have an else or an else if, you should be using braces. This is definitely a slippery slope, see @sergioramos's comment above about possible security issues.
| if (fileA.children === null) return -1; | ||
| if (fileB.children === null) return 1; | ||
| return fileA.name?.localeCompare(fileB.name || '') || 0; | ||
| if (fileA.children === undefined) { |
There was a problem hiding this comment.
This is @typescript-eslint/no-unnecessary-condition and seems to be a real issue (the comparison should be against undefined, not null).
| @@ -17,22 +17,32 @@ export function CenterPaneView({ file, ...props }: CenterPaneViewProps) { | |||
| if (file && isTipTap(file)) { | |||
| setLoading(true); | |||
| (async () => { | |||
| const content = await readFile(file); | |||
| const textContent = new TextDecoder('utf-8').decode(content); | |||
| const newBytes = await readFile(file); | |||
| onClick?.(selected); | ||
| } | ||
| }) | ||
| .catch((e) => { |
There was a problem hiding this comment.
sergioramos
changed the title
See #18, follow-on to #24
These are based on rules I've used on projects in the past. It's a mix of style conventions (
curly), nudges towards good modern JS style (prefer-destructuring,prefer-optional-chain), pushes towards better TypeScript types and some rules around using Promises correctly. I've also included a rule to sort JSX props which I've found very convenient.Most of these are auto-fixable.