-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Add reasonable linter configuration and lint everything #87
base: staging
Are you sure you want to change the base?
Conversation
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.
LGTM
Yeah that'll do |
packages/react-app/.eslintrc.js
Outdated
"no-console": "warn", | ||
"no-undef": "warn", | ||
"react-hooks/rules-of-hooks": "warn", |
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.
Turned these rules down to 'warn' instead of 'error'.
I think we can merge |
packages/react-app/.eslintrc.js
Outdated
"no-restricted-syntax": "off", | ||
"no-plusplus": "off", | ||
"no-console": "warn", | ||
"no-undef": "warn", |
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.
This should be an error as well.
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 thing about no-undef
is that it even complains about Promise
being undefined, which is a global
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.
@Dhaiwat10 Yeah, that can be resolved by adding the es6
parameter like this:
env: {
browser: true,
es6: true
},
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.
Cool. What about the console.logs? I don't think throwing an error for them is a good idea
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.
Everything else is okay, just for no-undef
:)
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.
Done
@Dhaiwat10 I am just leaving this open so that we can merge #70 first (I have my attention on here too :) |
There is no reason for us to have an absurd linter configuration that ignores most technical debt. I am using the one that Next.js officially recommends (which is
eslint-config-next
and incorporates the React plugins as well.✖ 163 problems (147 errors, 16 warnings)
We should probably focus on fixing the errors first, the warnings are mostly trivial.To run tests, you can do:
npx eslint --fix . OR npx next lint