Skip to content
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: move module declarations for non-route components to theme-classic #6629

Merged
merged 1 commit into from
Feb 7, 2022

Conversation

Josh-Cena
Copy link
Collaborator

@Josh-Cena Josh-Cena commented Feb 7, 2022

Motivation

Continuation of #6175. This fixes #6262 hopefully, and even if it doesn't, it's the best we can do. The theme aliases will be removed in the near future altogether, but for now, we will just put the components that the plugin directly references in the plugin typedef.

Have you read the Contributing Guidelines on pull requests?

Yes

@Josh-Cena Josh-Cena added the pr: maintenance This PR does not produce any behavior differences to end users when upgrading. label Feb 7, 2022
@facebook-github-bot facebook-github-bot added the CLA Signed Signed Facebook CLA label Feb 7, 2022
@netlify
Copy link

netlify bot commented Feb 7, 2022

✔️ [V2]

🔨 Explore the source changes: bff1187

🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/62010c5bf97c180008646ce8

😎 Browse the preview: https://deploy-preview-6629--docusaurus-2.netlify.app

@github-actions
Copy link

github-actions bot commented Feb 7, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🟠 Performance 78
🟢 Accessibility 100
🟢 Best practices 100
🟢 SEO 100
🟢 PWA 92

Lighthouse ran on https://deploy-preview-6629--docusaurus-2.netlify.app/

@Josh-Cena Josh-Cena merged commit 5db848f into main Feb 7, 2022
@Josh-Cena Josh-Cena deleted the jc/move-typedef branch February 7, 2022 12:17
@github-actions
Copy link

github-actions bot commented Feb 7, 2022

Size Change: 0 B

Total Size: 771 kB

ℹ️ View Unchanged
Filename Size
website/.docusaurus/globalData.json 47.2 kB
website/build/assets/css/styles.********.css 105 kB
website/build/assets/js/main.********.js 582 kB
website/build/index.html 36.7 kB

compressed-size-action

@@ -43,6 +43,21 @@ declare module '@theme/BlogListPaginator' {
export default BlogListPaginator;
}

declare module '@theme/BlogSidebar' {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand this. change but not 100% satisfied either

We should find a way to decouple theme/plugin, and yet find a way to ensure correct end-to-end type-safety between theme/plugin

Copy link
Collaborator Author

@Josh-Cena Josh-Cena Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The plugin has absolutely no knowledge of @theme/BlogSidebar, so it should definitely be in the theme's type def. Same with the others—I don't want to couple the theme design to the classic architecture too much, but delegate a lot of the freedom to the implementer side. FWIW, I even think each theme should have its unique themeConfig options.

Copy link
Collaborator

@slorber slorber Feb 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see

The BlogPostPage uses types from @docusaurus/plugin-content-blog so this looks fine/safe.

What surprised me is that it's the BlogSidebar that declares the sidebar prop, that type is then imported inside BlogPostPage

I'm not 100% which of those 2 components should "own" the type declaration (not sure if I'm clear 🤪 )

@Josh-Cena Josh-Cena mentioned this pull request Feb 10, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed Signed Facebook CLA pr: maintenance This PR does not produce any behavior differences to end users when upgrading.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version 2.0.0-beta.14 fails type check for typecsript
3 participants