-
-
Notifications
You must be signed in to change notification settings - Fork 8.6k
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
chore(v2): fix several lint warnings, add missing types, cleanup #3844
Conversation
Deploy preview for docusaurus-2 ready! Built without sensitive environment variables with commit e98959f |
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
Most of this looks like, but we'd rather avoid TS enums, and hasOwnProperty is probably fine to use?
@@ -18,21 +18,22 @@ const { | |||
} = getFileLoaderUtils(); | |||
|
|||
const createJSX = (node, pathUrl) => { | |||
node.type = 'jsx'; | |||
node.value = `<img ${node.alt ? `alt={"${escapeHtml(node.alt)}"} ` : ''}${ | |||
const jsxNode = node; |
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.
why not rename the fn param directly?
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.
no-param-reassign
rule - https://eslint.org/docs/rules/no-param-reassign
? sameProperties.filter((p) => !p.hasOwnProperty('important')) | ||
? sameProperties.filter( | ||
(p) => !Object.prototype.hasOwnProperty.call(p, 'important'), | ||
) |
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.
that feels a bit verbose, wonder if we shouldn't just disable this eslint rule instead?
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 it's fair to disable it, as much as leaving it on. The side effects or DDoS issues mentioned on the rule page seems not be related to the way those methods are used in the Docusuaurs code, but in the other hand I think that the slightly more verbose syntax won't hurt anyone, and there are only a few usages of hasOwnProperty
in the whole codebase.
priority: 0.5, | ||
trailingSlash: false, | ||
}; | ||
|
||
export const PluginOptionSchema = Joi.object({ | ||
cacheTime: Joi.number().positive().default(DEFAULT_OPTIONS.cacheTime), | ||
changefreq: Joi.string() | ||
.valid('always', 'hourly', 'daily', 'weekly', 'monthly', 'yearly', 'never') | ||
.valid(...Object.values(EnumChangefreq)) |
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.
not very fan of enums (non-std js), and not alone
https://fettblog.eu/tidy-typescript-avoid-enums/
https://www.techatbloomberg.com/blog/10-insights-adopting-typescript-at-scale/
I think we'd rather avoid using them when possible, even if the underlying lib does use them
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.
Yeah, I'm also not a big fan of Enums, but the String variety isn't that bad as for example, heterogeneous or ambient enums.
Of course I can also change back the types. Would you like to revert EnumChangefreq
usage completely or only the Enum values spread?
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.
let's keep it, just hope libs will stop exporting enums in the future :)
Motivation
This is cleanup PR which includes the following changes:
Have you read the Contributing Guidelines on pull requests?
Yes.
Test Plan
Build & run of Docusaurs 2 website locally.
Related PRs
No.