rusackas commented on PR #40799:
URL: https://github.com/apache/superset/pull/40799#issuecomment-4654997203
I dug into this one to see whether it could be made landable. Short answer:
**not as a mechanical Dependabot bump** — `react-router-dom` v5→v7 is
effectively a full rewrite (v5→v6 changed nearly every API; v7 then dropped
`react-router-dom` in favor of `react-router` and made the old `future` flags
mandatory). I've rebased the branch onto current `master` so the dep bump is no
longer stale, but I'm intentionally **not** force-pushing a code conversion,
because there is no compilable partial subset (more on that below).
### Why this is all-or-nothing
v7's `react-router-dom` is just `export * from 'react-router'` and the v5
symbols we rely on are gone entirely: `Switch`, `Redirect`, `useHistory`, and
`withRouter` no longer exist. The instant the resolved dependency is v7,
**every** importing file fails at module resolution simultaneously, so you
can't convert "half" the files and still have the bundle build. A coherent
landing has to convert all call sites in one atomic change.
### Scope (call sites in `superset-frontend/src`)
- **86 files** import from `react-router(-dom)` — **58 production**, **28
test**.
- `useHistory` → `useNavigate`: **38 files** (30 prod / 8 test). Underlying
method usage: `history.push` ×41, `history.replace` ×15, `history.location`
×10, `history.goBack`/`history.go` ×2.
- `<Switch>` → `<Routes>` + `<Route component=>`/`children` → `<Route
element={}>`: central setup in `src/views/App.tsx` and `src/embedded/index.tsx`.
- `<Redirect>` → `<Navigate>`: `src/SqlLab/components/App/index.tsx` (note
the default flips from `replace` to `push`).
- `withRouter` → hooks: 2 prod files (+ `RouteComponentProps` types).
- `<NavLink activeClassName>` → `className={({isActive})=>…}`: 1 file.
- `<Router history={memoryHistory}>` test-wrapper pattern (gone in v6+):
**5+ test files plus the shared `spec/helpers/testing-library.tsx`** — all need
`MemoryRouter`/`createMemoryRouter`.
### The two parts that are *not* mechanical (real rewrites)
1. **`history.block()` / `history.listen()` have no v6/v7 equivalent.**
`src/hooks/useUnsavedChangesPrompt/index.ts` (the unsaved-changes guard) uses
`history.block`, and `src/pages/Chart/index.tsx` uses `history.listen`.
Replacing these means `useBlocker`, which requires a **data router**
(`createBrowserRouter` + `RouterProvider`) — i.e. converting our top-level
`<BrowserRouter>` + route-array setup in `App.tsx` to a data-router config.
That's an architectural change, not a find-and-replace.
2. **Exact-matching default.** v5 `<Route path>` matched by prefix; v6/v7
match exactly. Our routes (e.g. `/dashboard/list/`, `/chart/add`) and the
`isFrontendRoute` prefix logic in `src/views/routes.tsx` need `/*` splats and
verification per route, and v7's mandatory `v7_relativeSplatPath` changes
relative-link resolution under splats.
### Other required cleanups
- Drop `@types/react-router-dom@^5.3.3` (devDep) — v7 ships its own types
and the v5 types will conflict.
- The direct `history@^5.3.0` dep is used mostly for the
`History`/`Location`/`Action` types (`hydrate.ts`, `chartAction.ts`,
`useUnsavedChangesPrompt`, `pages/Chart`); re-point these to v7's own types or
keep `history` only as a type source.
- v7 wants Node 20+/React 18+ (we're on React 18, fine).
### Suggested path to actually land it
1. **PR 1 (prep, lands today):** bump to `react-router-dom@6` with all
`future` flags on, do the mechanical codemods (`Switch`→`Routes`,
`component`→`element`, `useHistory`→`useNavigate`, `Redirect`→`Navigate`,
`withRouter`→hooks, NavLink, and the test `<Router history>`→`MemoryRouter`
wrappers incl. the shared test helper), and migrate `App.tsx`/`embedded` to a
data router so `useBlocker` replaces `history.block`/`listen`.
2. **PR 2 (trivial):** bump 6→7, flip imports to `react-router`, drop
`@types/react-router-dom`. With the v6 `future` flags already enabled, this
step is nearly a no-op.
I'd rather split it this way than drop one giant high-risk diff on top of
the routing core. Happy to take the v6 prep PR myself. Leaving this PR open
with the rebased bump for now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
