Strictify TypeScript across viewer code – eliminate any usages and export missing prop types#719
Strictify TypeScript across viewer code – eliminate any usages and export missing prop types#719rushabhcodes wants to merge 1 commit intotscircuit:mainfrom
Conversation
…y and consistency
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR aims to “strictify” TypeScript types across the CAD viewer by exporting the previously-internal prop types for the JSCAD and Manifold viewer components, introducing a shared CadViewerProps type for the main CadViewer, and tightening the camera session helpers/context API to use ThreeOrbitControls instead of any.
Changes:
- Export
PropsfromCadViewerJscadandCadViewerManifoldPropsfromCadViewerManifold. - Introduce
CadViewerPropsinCadViewerand replaceanyusage in viewer component signatures. - Replace
anywithThreeOrbitControlsfor camera session persistence APIs and their context typing.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/contexts/CameraControllerContext.tsx |
Tightens camera session function/context types to ThreeOrbitControls rather than any. |
src/CadViewerManifold.tsx |
Exports CadViewerManifoldProps for reuse by the main viewer component. |
src/CadViewerJscad.tsx |
Exports Props for reuse as JscadProps elsewhere. |
src/CadViewer.tsx |
Introduces CadViewerProps and replaces any, but currently relies on type assertions when passing props to each engine. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import type { Props as JscadProps } from "./CadViewerJscad" | ||
| import type { CadViewerManifoldProps } from "./CadViewerManifold" | ||
|
|
||
| export type CadViewerProps = JscadProps | CadViewerManifoldProps | ||
|
|
||
| const CadViewerInner = (props: CadViewerProps) => { |
There was a problem hiding this comment.
CadViewerProps uses Props from CadViewerJscad directly, but CadViewerJscad is declared as React.PropsWithChildren<Props>. As a result, CadViewer/CadViewerInner props currently don’t properly model the children prop for the JSCAD path (e.g. <CadViewer soup={...}>{...}</CadViewer> won’t type-check). Consider defining the JSCAD side as React.PropsWithChildren<JscadBaseProps> (or exporting a JscadProps type that already includes children) before building CadViewerProps.
| <CadViewerJscad | ||
| {...props} | ||
| {...(props as JscadProps)} | ||
| autoRotateDisabled={props.autoRotateDisabled || !autoRotate} | ||
| cameraType={cameraType} | ||
| onUserInteraction={handleUserInteraction} | ||
| onCameraControllerReady={handleCameraControllerReady} | ||
| /> | ||
| ) : ( | ||
| <CadViewerManifold | ||
| {...props} | ||
| {...(props as CadViewerManifoldProps)} | ||
| autoRotateDisabled={props.autoRotateDisabled || !autoRotate} | ||
| cameraType={cameraType} |
There was a problem hiding this comment.
The new CadViewerProps union is immediately bypassed via casts (props as JscadProps / props as CadViewerManifoldProps) when spreading into each engine component. This defeats the goal of eliminating any because the compiler can no longer ensure each branch receives the required props (notably CadViewerManifoldProps requires circuitJson or children). Prefer a discriminated union (e.g. an explicit engine prop) or a shared normalized prop shape so the branch props can be passed without type assertions.
This pull request improves type safety and code clarity in the CAD viewer codebase by introducing and exporting explicit prop types for the main viewer components and updating function signatures to use more specific types. The changes also update the context API to reflect these improvements.
Type safety and prop type improvements:
CadViewerPropsas a union ofJscadPropsandCadViewerManifoldProps, and updated theCadViewerandCadViewerInnercomponents to use this type instead ofany(src/CadViewer.tsx). [1] [2]Propsinterface fromsrc/CadViewerJscad.tsxfor use asJscadPropselsewhere in the codebase.CadViewerManifoldPropstype fromsrc/CadViewerManifold.tsxfor use in the main viewer component.Function signature and context API updates:
saveCameraToSessionandloadCameraFromSessionfunctions insrc/contexts/CameraControllerContext.tsxto require aThreeOrbitControlstype for thecontrolsparameter instead ofany, and updated the context value accordingly. [1] [2] [3]These changes improve code maintainability and reduce the risk of runtime errors by making prop and function types more explicit throughout the codebase.