chore: DocsPage to TypeScript #337
Conversation
🦋 Changeset detectedLatest commit: 98e21c7 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/hashicorp/react-components/Ucq7axZsYbnGRTPxoTdxUBrTjBkL |
packages/docs-page/index.tsx
Outdated
| } | ||
| } | ||
|
|
||
| const DocsPage: ComponentType<DocsPageProps> = ({ |
There was a problem hiding this comment.
As DocsPage doesn't accept children, I'm not sure it makes sense to use ComponentType here, which defines props as PropsWithChildren<DocsPageProps>. I would maybe just type props here:
export default function DocsPage({ ... }: DocsPageProps) {There was a problem hiding this comment.
I'll opt for your suggestion, but will need to add an explicit return type : JSX.Element
We have a linting rule — Missing return type on function. eslint@typescript-eslint/explicit-module-boundary-types — that warns if you don't have an explicit return type.
- 💭 Maybe we should re-assess this rule? Type-inference is pretty good, and sometimes explicit typing is prone to human error — 0 warnings, but incorrect type
There was a problem hiding this comment.
Ah yeah, that rule! I think the value of that rule is debatable for react components, but it can be handy for other methods. We have had a few discussions about this rule in the past 💭 cc @hashicorp/web-platform
I think the return type should be React.ReactElement, FWIW. This matches the definition of the component types from @types/react.
There was a problem hiding this comment.
I was looking at React.ReactElement (from the Sourcegraph link) ... vs. JSX.Element (from type-hint/inference) 😅
I'll update to React.ReactElement!
packages/docs-page/components.ts
Outdated
| ) { | ||
| productName: string, | ||
| additionalComponents: MDXProviderComponentsProp = {} | ||
| ): Record<string, string> { |
There was a problem hiding this comment.
defaultMdxComponents returns a map of string to components I think 💭
https://github.com/hashicorp/nextjs-scripts/blob/main/packages/docs-mdx/index.jsx#L11
There was a problem hiding this comment.
I'll do:
Record<string, (p: any) => JSX.Element>There was a problem hiding this comment.
Updated to
Record<string, (p: any) => ReactElement>- improve DocsPage typing - enable `className` prop on SearchBar
| const isMobile = useIsMobile() | ||
|
|
||
| return ( | ||
| <Search | ||
| className={className} |
🎟️ Asana Task
🔍 Preview Link
Description
This converts
DocsPageto TypeScript;No new changes to the component API!
PR Checklist 🚀
Items in this checklist may not may not apply to your PR, but please consider each item carefully.