-
-
Notifications
You must be signed in to change notification settings - Fork 8.5k
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
ESLintify Part 1 #837
ESLintify Part 1 #837
Conversation
Deploy preview for docusaurus-preview ready! Built with commit cae7d27 |
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 for doing this. I had few comments though
</a> | ||
); | ||
}); | ||
const showcase = siteConfig.users.filter(user => user.pinned).map(user => ( |
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.
Instead of filter and then map, I think using reduce
can be 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.
I'd have to use an if
inside reduce
, which makes the code less declarative, so I don't think it's a good idea.
const glob = require('glob'); | ||
const mkdirp = require('mkdirp'); | ||
const nodePath = require('path'); |
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.
Is there a reason why we use nodePath
instead of path
? I saw path
being used in another file. Is there conflicting name here ?
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.
Yep, you're right
lib/rename-version.js
Outdated
@@ -123,7 +124,7 @@ if (fs.existsSync(currentSidebarFile)) { | |||
fs.renameSync(currentSidebarFile, newSidebarFile); | |||
let sidebarContent = fs.readFileSync(newSidebarFile, 'utf8'); | |||
sidebarContent = sidebarContent.replace( | |||
new RegExp(`version-${RegExp.escape(currentVersion)}-`, 'g'), | |||
new RegExp(`version-${escapeRegexp(currentVersion)}-`, 'g'), |
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 we can escape it using a library instead of our own escape function. Its alrd in our dependencies
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'll look into using escape-string-regexp
lib/core/Head.js
Outdated
@@ -12,8 +12,10 @@ class Head extends React.Component { | |||
render() { | |||
const links = this.props.config.headerLinks; | |||
let hasBlog = false; | |||
links.map(link => { | |||
if (link.blog) hasBlog = true; | |||
links.forEach(link => { |
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.
const hasBlog = links.some(link => link.blog);
Hi 👋 I think there was a build failure from this. Syntax error? Maybe we need to upgrade our Circle build infra to support new constructs? Or there was a genuine error 😄 Sent with GitHawk |
Hmm odd. I'll look into it. |
Motivation
Enable some ESLint rules by making the code ESLint-compliant.
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
Local + Netlify.
Related PRs
#836