Update: React router dom from v5 to v6 #2580

Zen-cronic · 2025-01-10T16:08:52Z

Which problem is this PR solving?

Part of [Deps] Upgrade react-router-dom to 7.x #2532 & Upgrade react-router-dom to latest version #1825 (duplicate issues?)
Bridge towards v7 migration

Description of the changes

Incremental Migrate to v6: migration guide: Official v5 to v6 Migration Guide remix-run/react-router#8753
remove unused modules/dependencies

How was this change tested?

npm run test

Commands Ran

npm install @types/history@^4.7.11, removed in [deps] Remove @types/moment, @types/history and @types/classnames #1637 but added back b/c the pkg became fully typed only in v5.0.0
npm uninstall react-router-dom-v5-compat to uninstall compat pkg
npm install react-router-dom@^v6.28.1 to install v6
npm uninstall @types/react-router-dom b/c the rrd pkg became fully typed in v6.0.0

Checklist

I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md
I have signed all commits
I have added unit tests for the new functionality
I have run lint and test steps successfully
- for jaeger: make lint test
- for jaeger-ui: npm run lint and npm run test

Signed-off-by: Kaung Zin Hein <[email protected]>

This reverts commit 154c7c0. Signed-off-by: Kaung Zin Hein <[email protected]>

Signed-off-by: Kaung Zin Hein <[email protected]>

packages/jaeger-ui/src/components/App/index.test.js

Signed-off-by: Kaung Zin Hein <[email protected]>

codecov · 2025-01-13T14:57:22Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.58%. Comparing base (1ee19c2) to head (4228454).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2580      +/-   ##
==========================================
- Coverage   96.58%   96.58%   -0.01%     
==========================================
  Files         255      255              
  Lines        7732     7728       -4     
  Branches     1944     1996      +52     
==========================================
- Hits         7468     7464       -4     
  Misses        264      264

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-13T16:29:35Z

Migration to v6 done! All the unit tests pass

TODOs:

fix error on "Compare" page (TraceDiff):
Error: HTTP Error: strconv.ParseUint: parsing "...": invalid syntax
URL: /api/traces/...
Response body: { "data": null, "total": 0, "limit": 0, "offset": 0, "errors": [ { "code": 400, "msg": "strconv.ParseUint: parsing \"...\": invalid syntax" } ] }

Or when two traces are selected:
Error: HTTP Error: TraceID cannot be longer than 32 hex characters: 9a344d0e849454d3c184becc70ea8d1d...28eba2ae54dcf6b5cb1f9541926bc7ab
URL: /api/traces/9a344d0e849454d3c184becc70ea8d1d...28eba2ae54dcf6b5cb1f9541926bc7ab
Response body: Response body { "data": null, "total": 0, "limit": 0, "offset": 0, "errors": [ { "code": 400, "msg": "TraceID cannot be longer than 32 hex characters: 9a344d0e849454d3c184becc70ea8d1d...28eba2ae54dcf6b5cb1f9541926bc7ab" } ]

Interestingly, manually going to http://localhost:5173/trace/ does not throw; it shows the expected ui component.

Problem found: react-router dropped regex path support in v6

~~Continue v7 migration~~ Separate PR

yurishkuro · 2025-01-13T17:54:18Z

please do not bundle 6 & 7 migration into one PR, let's get a green CI and merge

yurishkuro · 2025-01-13T17:56:10Z

please document in the description how the dependency files were changed (which commands did you run). For example, why are "peer": true entries being removed? Are you using a different version of npm?

Signed-off-by: Kaung Zin Hein <[email protected]>

yurishkuro · 2025-01-13T18:21:32Z

packages/jaeger-ui/src/components/App/TraceIDSearchInput.test.js

@@ -29,9 +29,9 @@ describe('<TraceIDSearchInput />', () => {
    history = createMemoryHistory();
    render(
      <HistoryProvider history={history}>


why in packages/jaeger-ui/src/components/App/index.jsx the history provider is removed but here it's not? What is the motivation for either decision?

In packages/jaeger-ui/src/components/App/index.jsx, HistoryProvider is removed b/c rrd v6 works without being wrapped with that component.

But in this test and other tests, the history object from it's namesake package is used for assertions. So, removing HistoryProvider would mean a rewrite for these tests.

but what's the story with the history package, aren't we going to have to remove it altogether as incompatible going forward? It's ok if that clean-up can be done incrementally, we can defer it to other PRs.

history is no longer a peer dep in v6 and later. And yes, we should have a separate PR for removing it from this repo.

And we need to consider how to replace the function of that package - currently, it's used to provide the navigation props (e.g., history.push(/)) in class-based components wrapped with withRouteProps. Because they're class based, we can't use hooks like useNavigation). This discussion is better continued in another PR :)

Specifically, the migration guide you linked recommends using navigate instead of history.

And the docs say BrowserRouter incorporates the history in it.

Specifically, the migration guide you linked recommends using navigate instead of history.

yep, we should replace history with navigate from the useNavigate hook. But that change can't be made unless the class-based components (like this) are rewritten into functional components.

Signed-off-by: Kaung Zin Hein <[email protected]>

packages/jaeger-ui/src/components/App/index.jsx

packages/jaeger-ui/package.json

Zen-cronic · 2025-01-13T20:45:35Z

please document in the description how the dependency files were changed (which commands did you run). For example, why are "peer": true entries being removed? Are you using a different version of npm?

noted. i'm on node v22.12.0 and npm 10.9.0.
The peer:true entries seem to be related to theantd library

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-13T23:25:16Z

packages/jaeger-ui/src/components/App/index.jsx

+          <Routes>
+            <Route path="/" element={<Page />}>
+              <Route index element={<SearchTracePage />} />
+              <Route path={searchPath} element={<SearchTracePage />} />


should use relative paths in v7 migration: e.g., search instead of /search

packages/jaeger-ui/src/components/App/Page.tsx

yurishkuro · 2025-01-13T23:34:01Z

packages/jaeger-ui/src/components/App/index.jsx

@@ -42,8 +42,7 @@ import '../common/vars.css';
 import '../common/utils.css';
 import 'antd/dist/reset.css';
 import './index.css';
-import { history, store } from '../../utils/configure-store';
-import { HistoryProvider } from '../../utils/useHistory';


can this module be deleted?

not in this PR, other unrelated tests depend on it. We should remove history on its own PR

packages/jaeger-ui/src/components/SearchTracePage/index.test.js

packages/jaeger-ui/src/types/index.tsx

yurishkuro · 2025-01-13T23:38:48Z

package-lock.json

@@ -20103,7 +19951,7 @@
        "@ant-design/compatible": "^5.1.3",
        "@jaegertracing/plexus": "0.2.0",
        "@pyroscope/flamegraph": "0.21.4",
-        "@sentry/browser": "^8.48.0",
+        "@sentry/browser": "^8.18.0",


What causes this downgrade? Before you're running install/uninstall commands, are you on the latest set of dependencies (npm ci)?

it seems that the current dep version is 8.18.0?

jaeger-ui/packages/jaeger-ui/package.json

Line 51 in 1ee19c2

"@sentry/browser": "^8.18.0",

oh. I hate it when GH glitches like that. It's showing fake diffs, how to even trust its diff...

It is 8.48 in the lock file though:

$ grep sentry/browser package-lock.json "node_modules/@sentry/browser": { "resolved": "https://registry.npmjs.org/@sentry/browser/-/browser-8.48.0.tgz", "@sentry/browser": "^8.48.0",

in the original repo, the current versions in the lock file is 8.48.0 as well.

jaeger-ui/package-lock.json

Line 4121 in 1ee19c2

"version": "8.48.0",

so i updated the version in package.json to match that

yurishkuro

overall lgtm, but please address questions / comments.

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-14T01:25:53Z

packages/jaeger-ui/src/components/TraceDiff/url.tsx

-export const ROUTE_PATH = prefixUrl('/trace/:a?\\.\\.\\.:b?');
-
-const ROUTE_MATCHER = { path: ROUTE_PATH, strict: true, exact: true };
+export const ROUTE_PATH = prefixUrl('/trace-diff/:a?/:b?');


since regex route matching is dropped in rrd v6, a different approach is implemented.

a & b traces are optional dynamic parameters. They're prefixed with /trace-diff instead of /trace b/c /trace/:id is the pattern for TracePage

Zen-cronic · 2025-01-14T01:31:11Z

package-lock.json

@@ -2208,7 +2204,6 @@
      "resolved": "https://registry.npmjs.org/@ctrl/tinycolor/-/tinycolor-3.6.1.tgz",
      "integrity": "sha512-SITSV6aIXsuVNV3f3O0f2n/cgyEDWoSqtZMYiAmcsYHydcKrOz3gUxB/iXd/Qf08+IZX4KpgNbvUdMBmWz+kcA==",
      "license": "MIT",
-      "peer": true,


need be reverted?

yurishkuro

the Deep Dependency Graph on the Search page stopped working

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-14T02:36:40Z

the Deep Dependency Graph on the Search page stopped working

it might involve converting a few class-based components into functional ones; i'll keep exploring solutions

yurishkuro · 2025-01-14T03:47:29Z

If we have to, can we do it in a separate PR and merge it before this one?

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-16T23:25:19Z

the previous commit fixes the error of DDG not being displayed (due to a missing history object for navigation in the class components).

No components were rewritten into function components in this PR, but in this. The rewriting process would block the migration process b/c to use the rrdv6/7 hooks (i.e.g, useLocation, useNavigation) the components must be functional. So, a different workaround is used, explained below:

before, the class components (e.g., SearchTracePage, SearchResults, TracesDdgImpl) uses the redux state to keep track of the url params and the location object to check the current url.
However, after upgrading to rrdv6, the location object is not in sync with the url in the redux state.
So, i passed existing urlQueryParams props and/or a custom prop for the search url to the children components. This resulted in the SearchResults component correctly rendering the DDG:

But there are a few errors (so a few tests are failing):

Clicking on a node in the scatter plot should navigate to /trace/:id. The url does change but the respective component is not. That's because the TraceTimelineViewer (& related components) are reading from the redux state which isn't updated.

Using the Find search bar while on the DDG page erases the existing url params and removes the graph. This is because the location object that the search bar component reads from is not aware of the other url params:

jaeger-ui/packages/jaeger-ui/src/components/common/UiFindInput.tsx

Line 113 in 1ee19c2

const { uiFind: uiFindFromUrl } = parseQuery(state.router.location.search);

Clicking on the Find Traces form by default should set the view params to traces, not ddg:

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic · 2025-01-16T23:32:38Z

packages/jaeger-ui/src/components/App/index.jsx

@@ -89,47 +89,21 @@ export default class JaegerUIApp extends Component {
      <ConfigProvider theme={jaegerTheme}>
        <Provider store={store}>
          <HistoryProvider history={history}>


update: HistoryProvider is still needed by the router b/c history is used for navigation:

jaeger-ui/packages/jaeger-ui/src/components/SearchTracePage/SearchResults/index.tsx

Line 116 in 1ee19c2

history.push(getUrl({ ...urlState, view }));

useNavigate can't be used inside a class-based component.

Zen-cronic · 2025-01-16T23:34:33Z

packages/jaeger-ui/src/components/DeepDependencies/traces.tsx

@@ -36,7 +36,9 @@ const svcOp = memoizeOne((service, operation) => ({ service, operation }));

 // export for tests
 export function mapStateToProps(state: ReduxState, ownProps: TOwnProps): TReduxProps {
-  const urlState = getUrlState(ownProps.location.search);
+  const locationUrlState = getUrlState(ownProps.location.search);
+  const urlState = { ...locationUrlState, ...ownProps.urlQueryParams };


merging with the passed urlQueryParams b/c location and the redux state router are out of sync.

caveat of this approach: we need to do this to every component that are connected to the store and that affect the url in some way

Zen-cronic · 2025-01-16T23:36:50Z

...ger-ui/src/components/SearchTracePage/SearchResults/__snapshots__/DiffSelection.test.js.snap

@@ -93,7 +93,7 @@ exports[`DiffSelection renders multiple traces as expected 1`] = `
    className="DiffSelection--message"
  >
    <Link
-      to="/trace/trace-id-0...trace-id-1?cohort=trace-id-0&cohort=trace-id-1"
+      to="/trace-diff/trace-id-0/trace-id-1?cohort=trace-id-0&cohort=trace-id-1"


TODO: the second cohort param should be separated by an &

Zen-cronic · 2025-01-16T23:38:28Z

packages/jaeger-ui/src/components/DeepDependencies/traces.tsx

-    const urlArgs = queryString.parse(location.search);
+    const { navigateTo } = this.props;
+
+    const urlArgs = queryString.parse(navigateTo.split('/search?')[1]);


TODO fix:
parsing the manually passed url prop (that contains the correct params) like this is very hacky.

Zen-cronic · 2025-01-17T00:32:50Z

Blocked by #2588

Zen-cronic · 2025-01-18T16:58:09Z

more issue of useLocation and location not in sync

Zen-cronic added 6 commits January 10, 2025 10:48

Update: remove Redirect inside Switch

2af7813

Signed-off-by: Kaung Zin Hein <[email protected]>

Update: install react-router-dom v6 latest

154c7c0

Signed-off-by: Kaung Zin Hein <[email protected]>

Reinstall @types/history as some app modules depend on it

2455d90

Signed-off-by: Kaung Zin Hein <[email protected]>

Revert "Update: install react-router-dom v6 latest"

8a40827

This reverts commit 154c7c0. Signed-off-by: Kaung Zin Hein <[email protected]>

Replace Route with CompatRoute for incremental migration

2743cca

Signed-off-by: Kaung Zin Hein <[email protected]>

Update: Replace react-router-dom imports with compat imports

3ee6445

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic commented Jan 13, 2025

View reviewed changes

packages/jaeger-ui/src/components/App/index.test.js Outdated Show resolved Hide resolved

Merge branch 'main' into update/react-router-dom-v7

828e985

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic added 3 commits January 13, 2025 10:57

Update: Replace compat imports with react-router-dom-v6 imports

d5eb985

Signed-off-by: Kaung Zin Hein <[email protected]>

Fix: Use useLocation hook in functional TopNav component

93c2181

Signed-off-by: Kaung Zin Hein <[email protected]>

Fix: Snapshot for rrd-v6

52793ea

Signed-off-by: Kaung Zin Hein <[email protected]>

Fix: Lint tsc

aa98700

Signed-off-by: Kaung Zin Hein <[email protected]>

yurishkuro reviewed Jan 13, 2025

View reviewed changes

Update: Remove @types/react-router-dom

c1eaadd

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic commented Jan 13, 2025

View reviewed changes

packages/jaeger-ui/src/components/App/index.jsx Outdated Show resolved Hide resolved

Zen-cronic commented Jan 13, 2025

View reviewed changes

packages/jaeger-ui/package.json Show resolved Hide resolved

Update: Applied review changes

188ae81

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic changed the title ~~Update: react router dom from v5 to v7~~ Update: react router dom from v5 to v6 Jan 13, 2025

Zen-cronic changed the title ~~Update: react router dom from v5 to v6~~ Update: React router dom from v5 to v6 Jan 13, 2025

Update: Use Outlet in Page

8aebf4c

Signed-off-by: Kaung Zin Hein <[email protected]>

yurishkuro added changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements changelog:dependencies Update to dependencies and removed changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements labels Jan 13, 2025

Zen-cronic marked this pull request as ready for review January 13, 2025 23:18

Zen-cronic requested a review from a team as a code owner January 13, 2025 23:18

Zen-cronic requested review from pavolloffay and removed request for a team January 13, 2025 23:18

Zen-cronic commented Jan 13, 2025

View reviewed changes

yurishkuro reviewed Jan 13, 2025

View reviewed changes

packages/jaeger-ui/src/components/App/Page.tsx Show resolved Hide resolved

yurishkuro reviewed Jan 13, 2025

View reviewed changes

packages/jaeger-ui/src/components/SearchTracePage/index.test.js Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 13, 2025

View reviewed changes

packages/jaeger-ui/src/types/index.tsx Show resolved Hide resolved

yurishkuro reviewed Jan 13, 2025

View reviewed changes

Zen-cronic added 2 commits January 13, 2025 19:06

Update: Revert BrowserRouter usage in test

375bed2

Signed-off-by: Kaung Zin Hein <[email protected]>

Fix: Update regex route path matching to optional parameters

9e691d6

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic commented Jan 14, 2025

View reviewed changes

yurishkuro reviewed Jan 14, 2025

View reviewed changes

Fix: Update dep version to match lock file

4228454

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic mentioned this pull request Jan 16, 2025

Refactor: Rewrite search page related components into functional components #2588

Draft

4 tasks

Fix: Display Deep Dependency Graph on click

b0ac58a

Signed-off-by: Kaung Zin Hein <[email protected]>

Fix: Lint

fe98dc1

Signed-off-by: Kaung Zin Hein <[email protected]>

Zen-cronic commented Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update: React router dom from v5 to v6 #2580

Update: React router dom from v5 to v6 #2580

Zen-cronic commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

Zen-cronic commented Jan 13, 2025 •

edited

Loading

yurishkuro commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

yurishkuro Jan 13, 2025

Zen-cronic Jan 13, 2025

yurishkuro Jan 13, 2025

Zen-cronic Jan 13, 2025

yurishkuro Jan 13, 2025

yurishkuro Jan 13, 2025

Zen-cronic Jan 13, 2025

Zen-cronic commented Jan 13, 2025

Zen-cronic Jan 13, 2025

yurishkuro Jan 13, 2025

Zen-cronic Jan 13, 2025

yurishkuro Jan 13, 2025

Zen-cronic Jan 13, 2025

yurishkuro Jan 13, 2025

yurishkuro Jan 14, 2025

Zen-cronic Jan 14, 2025

yurishkuro left a comment

Zen-cronic Jan 14, 2025 •

edited

Loading

Zen-cronic Jan 14, 2025

yurishkuro left a comment

Zen-cronic commented Jan 14, 2025

yurishkuro commented Jan 14, 2025

Zen-cronic commented Jan 16, 2025

Zen-cronic Jan 16, 2025

Zen-cronic Jan 16, 2025 •

edited

Loading

Zen-cronic Jan 16, 2025

Zen-cronic Jan 16, 2025

Zen-cronic commented Jan 17, 2025

Zen-cronic commented Jan 18, 2025

Update: React router dom from v5 to v6 #2580

Are you sure you want to change the base?

Update: React router dom from v5 to v6 #2580

Conversation

Zen-cronic commented Jan 10, 2025 • edited Loading

Which problem is this PR solving?

Description of the changes

How was this change tested?

Commands Ran

Checklist

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

Zen-cronic commented Jan 13, 2025 • edited Loading

yurishkuro commented Jan 13, 2025

yurishkuro commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zen-cronic commented Jan 13, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Zen-cronic Jan 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

Zen-cronic commented Jan 14, 2025

yurishkuro commented Jan 14, 2025

Zen-cronic commented Jan 16, 2025

Choose a reason for hiding this comment

Zen-cronic Jan 16, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Zen-cronic commented Jan 17, 2025

Zen-cronic commented Jan 18, 2025

Zen-cronic commented Jan 10, 2025 •

edited

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

Zen-cronic commented Jan 13, 2025 •

edited

Loading

Zen-cronic Jan 14, 2025 •

edited

Loading

Zen-cronic Jan 16, 2025 •

edited

Loading