-
Notifications
You must be signed in to change notification settings - Fork 142
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
✨ [RUM-5500] React-router v7 support #3299
base: main
Are you sure you want to change the base?
Conversation
Bundles Sizes Evolution
🚀 CPU Performance
🧠 Memory Performance
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3299 +/- ##
=======================================
Coverage 93.70% 93.70%
=======================================
Files 290 294 +4
Lines 7652 7708 +56
Branches 1745 1758 +13
=======================================
+ Hits 7170 7223 +53
- Misses 482 485 +3 ☔ View full report in Codecov by Sentry. |
"react-router-6": "npm:[email protected]", | ||
"react-router-7": "npm:[email protected]" |
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.
💬 suggestion:
"react-router-6": "npm:[email protected]", | |
"react-router-7": "npm:[email protected]" | |
"react-router-dom-6": "npm:[email protected]", | |
"react-router-dom-7": "npm:[email protected]" |
import { | ||
createBrowserRouter as originalCreateBrowserRouter, | ||
createHashRouter as originalCreateHashRouter, | ||
createMemoryRouter as originalCreateMemoryRouter, | ||
} from 'react-router-7' | ||
import { startReactRouterView } from './startReactRouterView' | ||
|
||
type Router = ReturnType<typeof originalCreateBrowserRouter> | ||
|
||
export const createBrowserRouter: typeof originalCreateBrowserRouter = (routes, options) => | ||
registerRouter(originalCreateBrowserRouter(routes, options)) | ||
export const createHashRouter: typeof originalCreateHashRouter = (routes, options) => | ||
registerRouter(originalCreateHashRouter(routes, options)) | ||
export const createMemoryRouter: typeof originalCreateMemoryRouter = (routes, options) => | ||
registerRouter(originalCreateMemoryRouter(routes, options)) | ||
|
||
export function registerRouter(router: Router) { | ||
let location = router.state.location.pathname | ||
router.subscribe((routerState) => { | ||
const newPathname = routerState.location.pathname | ||
if (location !== newPathname) { | ||
startReactRouterView(routerState.matches) | ||
location = newPathname | ||
} | ||
}) | ||
startReactRouterView(router.state.matches) | ||
return router | ||
} |
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.
💬 suggestion: To help maintenance, we need to avoid code duplication as much as possible.
What about having a structure like this:
domain
reactRouter
startReactRouterView.ts
useRoutes.ts
createRouterts
routesComponent.ts
entries
reactRouterV6.ts
reactRouterV7.ts
Entry files would look like:
import { wrapCreateRouter } from '../domain/reactRouter/createRouter'
import { createRoutesComponent } from '../domain/reactRouter/routesComponent'
import {
createBrowserRouter as originalCreateBrowserRouter,
createHashRouter as originalCreateHashRouter,
createMemoryRouter as originalCreateMemoryRouter,
useRoutes as originalUseRoutes,
useLocation,
} from 'react-router-7'
export const createBrowserRouter = wrapCreateRouter(originalCreateBrowserRouter)
export const createHashRouter = wrapCreateRouter(originalCreateHashRouter)
export const createMemoryRouter = wrapCreateRouter(originalCreateMemoryRouter)
export const useRoutes = wrapUseRoutes({ useRoutes: originalUseRoutes, useLocation, ... })
export const Routes = createRoutesComponent(useRoutes)
And domain files would look like (types might be a bit harder to get well, let's see):
export function wrapCreateRouter(originalCreateRouter) {
return (routes, options) => registerRouter(originalCreateRouter(routes, options))
}
Motivation
Add support for react-router v7 for the rum-react plugin.
Changes
Reuse as much as possible the code from react-router-6, adapt unit test instead of cloning them
Testing
I have gone over the contributing documentation.