Refactor: Remove legacy components and migrate to TypeScript - #35
Refactor: Remove legacy components and migrate to TypeScript#35vinothkumar-onyx wants to merge 8 commits into
Conversation
Release 3.1.2 changes
- Deleted KAR, PublicHealthAuthority, and PublicHealthAuthorityList components. - Removed index.js and replaced with main.tsx for Vite compatibility. - Introduced serviceWorker.ts for service worker registration. - Added TypeScript configuration (tsconfig.json) and Vite configuration (vite.config.ts). - Updated withRouter to TypeScript. - Added setupTests.ts for testing library setup.
There was a problem hiding this comment.
Pull Request Overview
This pull request refactors the application by removing legacy components and migrating to TypeScript with Vite. The refactoring modernizes the codebase by replacing Create React App with Vite, converting JavaScript components to TypeScript, and removing outdated components.
Key changes include:
- Removed legacy JavaScript components (KAR, PublicHealthAuthority, PublicHealthAuthorityList)
- Replaced index.js with main.tsx for Vite compatibility
- Converted withRouter helper from JavaScript to TypeScript
- Added TypeScript configurations and reusable form components
Reviewed Changes
Copilot reviewed 83 out of 98 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Vite configuration with React plugin and test setup |
| src/withRouter.tsx | TypeScript version of router HOC |
| src/setupTests.ts | Test environment setup for TypeScript |
| src/serviceWorker.ts | Migrated service worker with TypeScript |
| src/main.tsx | New TypeScript entry point replacing index.js |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| onUpdate?: (registration: ServiceWorkerRegistration) => void; | ||
| }; | ||
|
|
||
| const isLocalhost = Boolean( |
There was a problem hiding this comment.
The old isLocalhost code is commented out instead of being removed, creating duplicate logic. Remove the commented-out code block (lines 13-135) to maintain code cleanliness.
… HealthCareSettings component
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 84 out of 99 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| const publicUrl = new URL(process.env.PUBLIC_URL!, window.location.href); | ||
|
|
||
| if (publicUrl.origin !== window.location.origin) { | ||
| return; | ||
| } | ||
|
|
||
| window.addEventListener("load", () => { | ||
| const swUrl = `${process.env.PUBLIC_URL}/service-worker.js`; |
There was a problem hiding this comment.
Using non-null assertion operator (!) on process.env.PUBLIC_URL without checking if it exists could cause runtime errors. The environment variable might be undefined in some environments.
| const publicUrl = new URL(process.env.PUBLIC_URL!, window.location.href); | |
| if (publicUrl.origin !== window.location.origin) { | |
| return; | |
| } | |
| window.addEventListener("load", () => { | |
| const swUrl = `${process.env.PUBLIC_URL}/service-worker.js`; | |
| const publicUrlValue = process.env.PUBLIC_URL || '/'; | |
| const publicUrl = new URL(publicUrlValue, window.location.href); | |
| if (publicUrl.origin !== window.location.origin) { | |
| return; | |
| } | |
| window.addEventListener("load", () => { | |
| const swUrl = `${publicUrlValue}/service-worker.js`; |
| reporter: ["text", "html"], | ||
| exclude: [ | ||
| "src/Services/AxiosConfig.ts", | ||
| "./vite.config.ts", |
There was a problem hiding this comment.
[nitpick] Including the Vite config file itself in the test coverage exclusion list is unnecessary since Vite configuration files are not typically part of test coverage analysis.
| "./vite.config.ts", |
| } | ||
|
|
||
| function handleUsernameChange(e: ChangeEvent<HTMLInputElement>): void { | ||
| const value = e.target.value.replace(/\s/g, ""); |
There was a problem hiding this comment.
Automatically removing all spaces from username and password inputs without user notification could lead to confusion if users intentionally include spaces. Consider providing visual feedback or validation messages instead.
| return; | ||
| } | ||
| function handlePasswordChange(e: ChangeEvent<HTMLInputElement>): void { | ||
| const value = e.target.value.replace(/\s/g, ""); |
There was a problem hiding this comment.
Automatically removing all spaces from username and password inputs without user notification could lead to confusion if users intentionally include spaces. Consider providing visual feedback or validation messages instead.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 84 out of 99 changed files in this pull request and generated 5 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| import { useNavigate } from "react-router-dom"; | ||
| import React from "react"; |
There was a problem hiding this comment.
[nitpick] The React import should come before the react-router-dom import for better organization and consistency with typical React import ordering conventions.
| import { useNavigate } from "react-router-dom"; | |
| import React from "react"; | |
| import React from "react"; | |
| import { useNavigate } from "react-router-dom"; |
| setTimeout(() => { | ||
| localStorage.removeItem("logoutSuccess"); | ||
| }, 1000); |
There was a problem hiding this comment.
[nitpick] The hardcoded 1000ms timeout should be extracted to a named constant for better maintainability and to make the timing intention clearer.
| setTimeout(() => { | ||
| localStorage.removeItem("sessionExpired"); |
There was a problem hiding this comment.
The timeout value 181 appears to be missing a closing parenthesis and the value seems arbitrary. This should be consistent with other timeout values and properly closed.
| onChange={(e: React.ChangeEvent<HTMLInputElement | HTMLSelectElement>) => { | ||
| if ( | ||
| type === "number" && | ||
| min !== undefined && | ||
| e.target.value !== "" && | ||
| !isNaN(parseFloat(e.target.value)) && | ||
| parseFloat(e.target.value) < min | ||
| ) { | ||
| const target = { ...e.target, value: String(min) }; | ||
| const syntheticEvent = { | ||
| ...e, | ||
| target, | ||
| } as React.ChangeEvent<HTMLInputElement | HTMLSelectElement>; | ||
| onChange(syntheticEvent); | ||
| return; | ||
| } | ||
| onChange(e); | ||
| }} |
There was a problem hiding this comment.
[nitpick] The synthetic event creation is complex and could be extracted to a separate helper function for better readability and testability.
| import * as path from "path"; | ||
|
|
||
| export default defineConfig({ | ||
| plugins: [react()], | ||
| resolve: { | ||
| alias: { | ||
| "@": path.resolve(__dirname, "src"), |
There was a problem hiding this comment.
[nitpick] Using import * as path is unnecessary when you only need the resolve method. Consider using import { resolve } from "path" for more specific imports.
| import * as path from "path"; | |
| export default defineConfig({ | |
| plugins: [react()], | |
| resolve: { | |
| alias: { | |
| "@": path.resolve(__dirname, "src"), | |
| import { resolve } from "path"; | |
| export default defineConfig({ | |
| plugins: [react()], | |
| resolve: { | |
| alias: { | |
| "@": resolve(__dirname, "src"), |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This pull request makes significant changes to modernize the build system, deployment, and environment configuration for the eCRNow-UI project. The main updates include migrating from Create React App to Vite, overhauling Docker and Nginx configuration, and updating documentation to reflect the new workflows. Several legacy files and configurations are removed, and new mock/test resources are added.
Build System and Tooling Modernization:
package.jsonand adding Vite-specific dependencies. The build, development, and test commands now use Vite and Vitest instead of react-scripts and Jest..env.examplewith Vite-compatible environment variables, replacing the oldenv.examplethat used React's naming conventions. [1] [2]index.htmlfor Vite, replacing the previous CRA-stylepublic/index.html. [1] [2]Docker and Deployment Changes:
Mock and Testing Resources:
mock/db.json,mock/json-server.json,mock/routes.json, andmock/server.js. [1] [2] [3] [4]db.jsonfor API testing and local development.Styling Improvements:
src/App.cssto add new CSS variables and improved form validation styles, aligning with the new primary color and validation feedback.Summary of Most Important Changes
Build System and Environment Configuration:
package.json, and switching environment variable usage to Vite's conventions (e.g.,VITE_ECR_BASE_URL). [1] [2] [3]public/index.htmlwith a new Vite-compatibleindex.html. [1] [2]Docker and Deployment:
Documentation:
Mock and Testing:
Styling:
src/App.css.