-
-
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
refactor: properly type docs version #5284
Conversation
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
✔️ [V2] 🔨 Explore the source changes: 478e3d2 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/610b4be36f94800008c62529 😎 Browse the preview: https://deploy-preview-5284--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5284--docusaurus-2.netlify.app/ |
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, would just like to avoid the useDocs type duplication
packages/docusaurus-theme-classic/src/theme/hooks/useContextualSearchFilters.ts
Outdated
Show resolved
Hide resolved
@@ -137,3 +137,32 @@ declare module '@theme/Seo' { | |||
const Seo: (props: Props) => JSX.Element; | |||
export default Seo; | |||
} | |||
|
|||
declare module '@theme/hooks/useDocs' { |
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.
All this is not in a very good state currently and I'd like to have content plugins expose more easily code/types/hooks to the theme.
I'd like to avoid such type duplication, as it's painful and error-prone to maintain.
What about creating an indexClient.ts
, exporting types that we need there/code/hooks, and migrate away from plugin-content-docs.d.ts
file? (ie also update package.types
=> indexClient.d.ts
)
If it's too complicated, one possibility could be to export the existing types instead of redeclaring them?
declare module '@theme/hooks/useDocs' {
export type * from "./theme/hooks/useDocs"
}
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 agree with your concerns about code duplication, but not sure I understood your first solution fully. Since plugin-content-docs.d.ts
contains almost all @theme/*
modules except the @docusaurus/plugin-content-docs-types
module, all the theme interfaces will still be exported from the same file, maybe just a renamed file.
Also, the typing system seems quite inconsistent across the content plugins. The pages plugin has a similar src/plugin-content-pages.d.ts
file, but the blog has an /index.d.ts
file. Still, the docs plugin is the only one exporting a theme hook, causing all these complications.
The second solution only works for the type exports, but most of the exports here are function exports. I don't know how to handle this elegantly either...
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.
Going to merge this for now and try to figure this out later ;)
packages/docusaurus-theme-classic/src/theme/DocVersionBanner/index.tsx
Outdated
Show resolved
Hide resolved
Signed-off-by: Josh-Cena <sidachen2003@gmail.com>
looks like there's an issue with this PR, prod categories can't uncollapse anymore due to process.env appearing in client-side bundle, investigating |
Yes, I've noticed this as well 😅 |
Motivation
Remove the TODO & Reduce ESLint warnings about
no-explicit-any
.There were far more implicit
any
's than captured by CI; for example, all theuseActivePlugin
hook usage was wrongly colored as green (for classes/types) rather than yellow (for functions) in my VSCode because the editor doesn't know what's the module@theme/hooks/useDocs
at all.Used some non-null assertions, but probably little side-effects (considering it's still an improvement from the previous
any
)Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
The build passes