-
-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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(module-type-aliases): remove fallback aliases #5726
Conversation
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
✔️ [V2] 🔨 Explore the source changes: a869d8c 🔍 Inspect the deploy log: https://app.netlify.com/sites/docusaurus-2/deploys/616bfd369fb3170007022d8c 😎 Browse the preview: https://deploy-preview-5726--docusaurus-2.netlify.app |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5726--docusaurus-2.netlify.app/ |
Signed-off-by: Josh-Cena <[email protected]>
Signed-off-by: Josh-Cena <[email protected]>
} | ||
|
||
declare module '@theme/NotFound' { | ||
export default function NotFound(props: any): JSX.Element; |
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.
Does the NotFound
actually receive props?
function NotFound() { |
return <NotFound {...props} />; |
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.
@slorber I actually haven't figured this out🤔 To prevent potential breaking changes I didn't remove the props, but from the implementation it seems there's no props
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.
Oh actually I think we should remove props here.
I think React-Router automatically injects some props in all the declared route components, but I'm not sure we should expose those in practice, as it couples too much to the routing system and in all cases, it's still possible to use React-Router hooks.
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.
Okay, makes sense. I guess the {...props}
on DocPage
was just a mistake (due to NotFound
not being properly typed before). Will be fixed later
Thanks 👍 makes sense |
Motivation
These are often loopholes for missing definitions and future bugs. To keep things strict, let's remove the legacy fallbacks.
Have you read the Contributing Guidelines on pull requests?
Yes
Test Plan
Build & typecheck still passes