-
-
Notifications
You must be signed in to change notification settings - Fork 8.4k
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
style(v2): reduce number of ESLint warnings #4993
Conversation
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
✔️ [V2] 🔨 Explore the source changes: 269c971 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/60d490fd4a60f80007cc98af 😎 Browse the preview: https://deploy-preview-4993--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4993--docusaurus-2.netlify.app/ |
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
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, not agreeing with everything but overwall happy to reduce eslint warnings
I personally like TS function return type inference but it seems to be a best practice to type return types explicitly so why not
@@ -127,7 +127,10 @@ function assertIsCategory( | |||
); | |||
} | |||
// "collapsed" is an optional property | |||
if (item.hasOwnProperty('collapsed') && typeof item.collapsed !== 'boolean') { | |||
if ( | |||
Object.prototype.hasOwnProperty.call(item, 'collapsed') && |
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.
too much ceremony, I'd just replace by typeof item.collapsed !== "undefined"
?
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 is actually a consideration about collapsed
existing on the prototype chain of item
. Is it safe to assume here that typeof item.collapsed !== "undefined"
works the same?
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.
yes it looks to me as the sidebar is a regular object
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 replace with typeof item.collapsed !== "undefined"
? it has not been done here
packages/docusaurus-theme-bootstrap/src/theme/ThemedImage/index.tsx
Outdated
Show resolved
Hide resolved
packages/docusaurus-theme-classic/src/theme/ThemedImage/index.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
…x.tsx Co-authored-by: Sébastien Lorber <slorber@users.noreply.github.com>
@slorber If I'm delving into typing (the |
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
smaller PRs are always easier to review/merge, so splitting the work is not a bad idea. |
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
@slorber I think I'm mostly done—reduced warning count by >50. The rest of them (33) contain:
docusaurus/packages/docusaurus-plugin-content-docs/src/sidebarItemsGenerator.ts Lines 270 to 274 in 7dc9fe8
Need some help here... Airbnb is complaining about await docs.reduce(
(p, doc) => p.then(() => handleDocItem(doc)),
Promise.resolve(),
);
I see that we are planning to add hotkey binding maps. This warning will remind us of this feature :P
|
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
Honestly don't like Airbnb preset, it's way too opinionated with weird rules, but it's the current docu preset so let's just disable what is annoying. This rule can also be disabled, it's not really applicable for NodeJS code IMHO. I'll merge this PR as is, any further improvements are welcome |
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.
finally, asking for a few changes 🤪
// eslint-disable-next-line no-await-in-loop | ||
await handleDocItem(doc); | ||
} | ||
await docs.reduce( |
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.
actually I'd prefer to revert this change
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.
Although I reverted this, I'm pretty confident that the new code works correctly😂 Just that it is less readable
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.
yes definitively works with reduce but much more complicated :)
@@ -127,7 +127,10 @@ function assertIsCategory( | |||
); | |||
} | |||
// "collapsed" is an optional property | |||
if (item.hasOwnProperty('collapsed') && typeof item.collapsed !== 'boolean') { | |||
if ( | |||
Object.prototype.hasOwnProperty.call(item, 'collapsed') && |
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 replace with typeof item.collapsed !== "undefined"
? it has not been done here
Okay cool. Lemme address those... |
This reverts commit c7525d4.
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
thanks 👍 |
Motivation
When I'm working on the project I was constantly overwhelmed (exaggeration here; there aren't so many) by ESLint warnings. Although they aren't serious issues, I decide it's better that we address these issues, since this is what we have the linter for.
There are some warnings that I probably need help with, especially regarding
any
types. (I realize how intractable typing objects can become.) I don't want to easily resort toeslint-disable
comments.Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
These are mostly style changes that don't really impact functionality to any degree.