refactor: minor ESLint improvements #5981
Conversation
Josh-Cena
added
the
pr: polish
|
✔️ [V2] 🔨 Explore the source changes: bf2f29b 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/61aa4279e8cc39000a284abd 😎 Browse the preview: https://deploy-preview-5981--docusaurus-2.netlify.app |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5981--docusaurus-2.netlify.app/ |
|
Size Change: -234 kB (-26%) 🎉 Total Size: 656 kB
ℹ️ View Unchanged
|
| themeConfig: ThemeConfig; | ||
| // Why partial? To make TS correctly figure out the contravariance in parameter. | ||
| // In practice it's always normalized | ||
| themeConfig: Partial<ThemeConfig>; |
There was a problem hiding this comment.
I don't understand what you mean here
There was a problem hiding this comment.
From the rule documentation of @typescript-eslint/method-signature-style:
Methods are always bivariant in their argument, while function properties are contravariant in their argument under
strictFunctionTypes.
Before, we had translateThemeConfig defined as method, so it's bivariant (essentially, see it as TS always doing type assertions—as long as the types fully overlap, the check passes).
Now, translateThemeConfig is a function. Functions are contravariant in their arguments, meaning that if we do...
const plugin: Plugin = themeClassic;...TS would expect Parameters<themeClassic['translateThemeConfig']> to be a supertype of Parameters<plugin['translateThemeConfig']>. This is much safer than bivariance because it should be what you expect for functions: a function foo that accepts Dog shouldn't be assignable to a function that accepts Animal because foo might call properties that only exist on Dog.
However, {themeConfig: import('@docusaurus/theme-common').ThemeConfig} is not a supertype of {themeConfig: Record<string, unknown>}, because the former requires properties like navbar, footer, etc., while the latter, which is the one defined in the core, expects none of the properties. That means themeClassic.translateThemeConfig can, theoretically, do something specific with these properties while the core doesn't know that when calling this method and pass in a theme config without footer, causing a "cannot read from undefined" error at runtime.
In practice, themeClassic.translateThemeConfig doesn't actually demand these properties to always exist, albeit they would always. Therefore making it Partial makes TS happy and also makes it make more sense.
There was a problem hiding this comment.
"Contravariance in argument" is quite hard to explain without using this jargon, so I decided to just write that one-line hint and expect people to have some sense of what's going on instead of being fully informed...
There was a problem hiding this comment.
I understand what is covariance/contravariance, it's just I don't really understand why it requires Partial here
In practice, this method receives a normalized/validated themeConfig. If validation ensures that themeConfig.navbar is mandatory, we shouldn't have to deal with a partial theme config and handle undefined that can't happen in practice.
The main problem is that we have no way for the theme to declare its ThemeConfig param, we should have a generic like Plugin<PluginContent = unknown,PluginThemeConfig = unknown> and get the right ThemeConfig type injected in the method.
Not sure how easy this refactor would be, but we should add more generic types someday.
In the meantime I'll just add an adapter layer:
Josh-Cena
added
the
status: awaiting review
| themeConfig: ThemeConfig; | ||
| // Why partial? To make TS correctly figure out the contravariance in parameter. | ||
| // In practice it's always normalized | ||
| themeConfig: Partial<ThemeConfig>; |
There was a problem hiding this comment.
I understand what is covariance/contravariance, it's just I don't really understand why it requires Partial here
In practice, this method receives a normalized/validated themeConfig. If validation ensures that themeConfig.navbar is mandatory, we shouldn't have to deal with a partial theme config and handle undefined that can't happen in practice.
The main problem is that we have no way for the theme to declare its ThemeConfig param, we should have a generic like Plugin<PluginContent = unknown,PluginThemeConfig = unknown> and get the right ThemeConfig type injected in the method.
Not sure how easy this refactor would be, but we should add more generic types someday.
In the meantime I'll just add an adapter layer:
Josh-Cena
added
pr: maintenance
Motivation
@typescript-eslint/method-signature-style, reason in that ruleHave you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Lint