Switch to typescript - #65
Conversation
There was a problem hiding this comment.
Pull request overview
This PR migrates the codebase from JavaScript to TypeScript to enable type generation for consumers and improve IDE/AI tooling support. The migration includes adding TypeScript configuration files, converting component files from .jsx to .tsx, adding type definitions, and updating the build process to generate declaration files.
Changes:
- Added TypeScript configuration (tsconfig.json, tsconfig.build.json) with strict mode enabled
- Converted 7 React components from .jsx to .tsx with TypeScript type definitions
- Updated build process to generate type declarations alongside compiled code
- Added ambient module declarations for Instructure UI libraries
Reviewed changes
Copilot reviewed 30 out of 33 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.json | Base TypeScript configuration with strict mode and React JSX support |
| tsconfig.build.json | Build-specific config to generate declaration files, excluding tests and stories |
| src/types/instructure-ui.d.ts | Ambient module declarations for Instructure UI packages without types |
| src/index.ts | Main entry point converted from .js, exports all components |
| src/components/tokenRetriever/LtiTokenRetriever.tsx | Converted with TypeScript types for props and state |
| src/components/modal/LtiLimitModal.tsx | Converted class component with TypeScript generics |
| src/components/heightLimit/LtiHeightLimit.tsx | Converted with complex type definitions for window messages |
| src/components/launchOAuth/LaunchOAuth.tsx | Converted with union type for server prop |
| src/components/errorBillboard/ErrorBillboard.tsx | Converted with simple prop types |
| src/components/applyTheme/LtiApplyTheme.tsx | Converted class component with async theme loading |
| src/components/LtiPageSettings/LtiPageSettings.tsx | Converted functional component with hooks |
| src/components/promptOAuth/PromptOAuth.jsx | DELETED WITHOUT REPLACEMENT |
| package.json | Added TypeScript and type packages, updated build script and exports |
| vite.config.js | Updated entry point from .js to .ts |
| Various test/story imports | Updated to remove .jsx extensions from imports |
Comments suppressed due to low confidence (4)
src/components/LtiPageSettings/LtiPageSettings.tsx:107
- The useEffect hook on line 59 has missing dependencies. The effect uses the
fetchThemefunction andlogDebugfunction which are not in the dependency array. While these functions are defined in the component body and may not change between renders in practice, React's exhaustive-deps rule would flag this. Consider either adding these to the dependency array, wrapping them in useCallback, or moving the fetchTheme function inside the useEffect to avoid potential stale closure issues.
src/components/tokenRetriever/LtiTokenRetriever.tsx:144 - Storage inconsistency: The saveJwt function saves the JWT to sessionStorage (line 134), but loadJwt tries to retrieve it from localStorage (line 144). These should use the same storage mechanism. This bug appears to have been introduced during the TypeScript conversion.
src/components/modal/LtiLimitModal.tsx:27 - PropTypes are incomplete and inconsistent with TypeScript types. The LtiLimitModalProps type includes onOpen and onClose (lines 12-13), but the static propTypes object only includes label and debug (lines 24-27). For consistency, either remove PropTypes entirely (since TypeScript provides compile-time type checking) or ensure they match the TypeScript types completely.
src/components/heightLimit/LtiHeightLimit.tsx:40 - Component state property 'debug' is written, but it is never read.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 30 out of 33 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
src/components/tokenRetriever/LtiTokenRetriever.tsx:90
- The useEffect hook at line 34 has an incomplete dependency array. It only includes ltiServer, but the effect also uses the location prop (through getToken and getServer functions). If the location prop changes, the effect should re-run to fetch a new token. Add location to the dependency array, or move getToken and getServer functions inside the useEffect to make the dependencies clear.
src/components/launchOAuth/LaunchOAuth.tsx:69 - A message event listener is added in componentDidMount but never removed in componentWillUnmount. This creates a memory leak if the component is unmounted and remounted multiple times. Add a componentWillUnmount method that removes the event listener, or store a reference to the handler function so it can be removed properly.
src/components/tokenRetriever/LtiTokenRetriever.tsx:144 - The loadJwt function reads from localStorage, but the saveJwt function writes to sessionStorage. This inconsistency will cause the JWT to never be found when attempting to load it, breaking the token retrieval functionality. Both functions should use the same storage mechanism (either both sessionStorage or both localStorage).
src/components/LtiPageSettings/LtiPageSettings.tsx:107 - The useEffect hook has an empty dependency array but uses fetchTheme, logDebug, and themeRetries inside it. These dependencies should be included in the dependency array, or the functions should be moved inside the useEffect, or wrapped with useCallback to avoid potential issues. React's exhaustive-deps eslint rule would flag this as a warning.
src/components/modal/LtiLimitModal.tsx:27 - The propTypes definition is incomplete. The TypeScript type LtiLimitModalProps includes onOpen, onClose, and children, but these are missing from the propTypes. While TypeScript provides type checking at compile time, PropTypes provide runtime validation which is valuable for a library. Either remove PropTypes entirely (since TypeScript now provides the types), or make them complete by including all props: onOpen, onClose, and children.
src/components/heightLimit/LtiHeightLimit.tsx:40 - Component state property 'debug' is written, but it is never read.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This allows us to easily output types for our components and improves IDE/AI use of the library. It also makes the library easier to use in an existing project that uses typescript.
|




This allows us to easily output types for our components and improves IDE/AI use of the library. It also makes the library easier to use in an existing project that uses typescript.