-
Notifications
You must be signed in to change notification settings - Fork 5
update packages #73
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
update packages #73
Conversation
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.
Pull Request Overview
This PR updates the SolidJS Router implementation to be compatible with a newer version of @solidjs/router, replacing the deprecated Routes component with Router and updating the API usage throughout the application.
- Updated
@solidjs/routerfrom v0.5.0 to v0.14.0 and other dependencies - Replaced
Routescomponent withRouteracross all route definitions - Changed
elementprop tocomponentprop for route definitions
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| package.json | Updated dependencies including @solidjs/router, solid-js, vite, and Node.js version |
| src/index.tsx | Replaced Routes with Router and removed unused Routes import |
| src/components/App.tsx | Updated router implementation and changed element prop to component |
| src/admin/index.tsx | Replaced Routes with Router in admin routing |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/index.tsx
Outdated
| </Routes> | ||
| <Route path="/admin/*" component={Admin} /> | ||
| {/* <Route path="/map" element={<Map />} /> */} | ||
| <Route path="/admin/*" component={Admin} /> |
Copilot
AI
Sep 20, 2025
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.
Duplicate route definition for '/admin/*'. The route on line 21 will never be reached as it's identical to line 19.
| <Route path="/admin/*" component={Admin} /> |
src/components/App.tsx
Outdated
| @@ -1,4 +1,4 @@ | |||
| import { Route, Routes, useLocation, useNavigate } from '@solidjs/router'; | |||
| import { Route, Router, useLocation, useNavigate } from '@solidjs/router'; | |||
Copilot
AI
Sep 20, 2025
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.
Importing Router is incorrect for nested routing. Since this component is already inside a Router (from index.tsx), you should use Routes instead of Router for nested route definitions.
src/components/App.tsx
Outdated
| <Router> | ||
| <Route path="/" component={() => <></>} /> |
Copilot
AI
Sep 20, 2025
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.
Using Router here creates a nested router instance which will break routing. This should be Routes to define nested routes within the existing router context.
| import { | ||
| Route, | ||
| Routes, | ||
| Router, |
Copilot
AI
Sep 20, 2025
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.
Importing Router is incorrect for nested routing. Since this is a component rendered within a route, you should import Routes instead of Router.
| Router, |
| return ( | ||
| <> | ||
| <Routes> | ||
| <Router> |
Copilot
AI
Sep 20, 2025
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.
Using Router here creates a nested router instance which will break routing. This should be Routes to define nested routes within the existing router context.
Uh oh!
There was an error while loading. Please reload this page.