react-router migration batch 3: Inventory and Setting screens#384
Closed
blaipr wants to merge 3 commits into
Closed
react-router migration batch 3: Inventory and Setting screens#384blaipr wants to merge 3 commits into
blaipr wants to merge 3 commits into
Conversation
Stage 2 of the React stack modernization. react-router-dom 6/7 removed
Switch, useHistory, useRouteMatch, Redirect and withRouter, which 243
files here still use, so the migration has to be incremental. This sets
up the official bridge and migrates the first screen:
- Add react-router-dom-v5-compat (ships router v6 alongside v5) and
mount CompatRouter inside the app's HashRouter, so v5 and v6 routing
contexts coexist and components can be migrated file by file.
- Wire the v6 context into testUtils/enzymeHelpers: the v5 Router keeps
its history subscription and drives re-renders, while a fully
controlled nested v6 Router (location from v5 context, navigator =
the shared history) provides the v6 context without a second
subscription - no act() warnings, and shallow rendering still works.
- Migrate screens/Login as the demonstration slice: Redirect becomes
Navigate (from the compat package), and the withRouter wrapper is
dropped (the component never used the injected props).
- Remove a test.only in Login.test.js that had silenced the other 14
Login tests since the initial import, and fix the two assertions that
had rotted while dark (the custom login screen has no LoginMainHeader;
branding-fetch errors fall back to the default logo without a modal).
The suite now runs 2869 tests, 14 more than before.
Migration recipe for follow-up PRs, per component:
useHistory().push(x) -> useNavigate(); navigate(x)
useRouteMatch().params -> useParams()
<Redirect to={x} /> -> <Navigate to={x} />
withRouter(C) -> hooks (or delete if props unused)
importing from 'react-router-dom-v5-compat'; the root Switch in App.js
migrates last, then react-router-dom flips to v6 and the compat package
is removed.
Migrates the two largest screen directories (74 files) to the
react-router-dom-v5-compat APIs using the established recipe
(useHistory -> useNavigate, history.location -> useLocation, params-only
useRouteMatch -> useParams, conditional Redirect -> Navigate; route
trees stay on v5 for the final phase).
Two findings specific to this batch:
- navigate() from the compat package is not referentially stable (it
depends on the current location), so redirect-on-result effects that
previously listed the stable v5 history object in their deps would
refire after unrelated navigations. The five affected useEffects
(SmartInventoryAdd/Edit, InventorySourceAdd/Edit, SubscriptionEdit)
drop navigate from their deps with an explanatory eslint-disable,
preserving the exact v5 behavior. Caught by the SmartInventoryAdd
cancel test: the post-submit redirect effect refired and overrode the
cancel navigation.
- UIEdit passed a custom hardReload field on the v5 location object;
v6 navigation only carries pathname/search/hash/state, so it moves to
navigation state (navigate(..., { state: { hardReload: true } })) and
UIDetail reads location.state.hardReload. history.go() with no
argument becomes navigate(0).
The ConstructedInventoryDetail RTL suite mounts with a bare v5 Router
and now needs the v6 context; its wrapper gains the controlled v6
Router layer.
This was referenced Jun 11, 2026
test_licenses requires every package.json dependency to have a matching file in licenses/ui (caught by an integration run of the full Python suite over all open PRs combined). (cherry picked from commit c8103b1)
This was referenced Jun 11, 2026
Contributor
Author
|
Superseded by #392, which unifies the v5-compat bridge and all three migration batches into a single PR (one squashed commit, identical content, full gates green). Done as part of making the whole queue conflict-free and mergeable in any order. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
📋 Merge instructions — all 13 open PRs, any order, zero conflicts
Every pair of open PRs has been verified conflict-free with
git merge-tree(0 conflicts across all 78 pairs), and the union of all 13 was integration-tested green (Python suite 3477 passed, Jest 2873 passed, lint, build, licenses). Merge in any order you like. Two recommendations, not requirements:The full queue:
SUMMARY
Third batch of the incremental react-router 5 → 6 migration: the two largest screen directories, Inventory and Setting (74 files), using the established recipe (
useHistory→useNavigate,history.location→useLocation(), params-onlyuseRouteMatch→useParams(), conditionalRedirect→Navigate; route trees stay on v5 for the final phase).Two findings worth reviewer attention:
navigate()from the compat package is not referentially stable (its identity changes with the location), unlike v5'shistoryobject. Redirect-on-result effects that listedhistoryin their deps would refire after unrelated navigations if mechanically converted tonavigate. The five affecteduseEffects (SmartInventoryAdd/Edit, InventorySourceAdd/Edit, SubscriptionEdit) dropnavigatefrom deps with an explanatoryeslint-disable, preserving exact v5 behavior. This was caught by an existing test — the post-submit redirect effect refired and overrode the cancel navigation.hardReloadflag on the v5 location object; it now travels in navigation state (navigate(..., { state: { hardReload: true } })) and UIDetail readslocation.state?.hardReload.history.go()becomesnavigate(0).Test changes: the ConstructedInventoryDetail RTL suite mounts with a bare v5 Router and gains the controlled v6 Router layer; UIEdit assertions read
location.state.Stacked on #381; parallel to #382 and #383 (disjoint files, any merge order once the bridge lands). Remaining after this: the route-tree phase (Switch/Route/match.path, prefix-match useRouteMatch, root Switch in App.js), then the v6 flip.
ISSUE TYPE
COMPONENT NAME
ASCENDER VERSION
ADDITIONAL INFORMATION
All UI gates run against this branch state:
Heads-up: the same navigate-in-deps pattern exists at three sites in #382/#383 (AddResourceRole, CredentialAdd, CredentialEdit); fix commits are being pushed to those branches.