Upgrade antd to 4.24.13 #1734

anshgoyalevil · 2023-08-25T11:26:21Z

Which problem is this PR solving?

This PR attempts to upgrade antd to v4.24.13 (Latest as of 28-08-2023

Description of the changes

AntD v4 requires a newer version of React typings, which also requires an explicit Yarn resolution to avoid pulling in other versions of the typings.
React refs for AntD Input components need a typing update.
The NumberInput component now guarantees to return number | null, so some custom type-checking code related to that can be removed.
Typing changed around the Table component around row selection and sorting logic, requiring appropriate updates. In particular, sorting by multiple sortable columns now reports all columns that are active in sorting, so the event handling code must also accept an array.
window.matchMedia API is not provided by jsdom, so custom mocking logic needs to be in place
dataIndex now requires an array of keys to work with tables with nested keys.
antd/compatible library is used to make forms work with the existing libraries
Tests are updated
Visual changes are verified

Most of the suggestions have been taken up from the PR #1261, thanks to @mszabo-wikia

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: yarn lint and yarn test

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil · 2023-08-25T11:34:02Z

...i/src/components/Monitor/ServicesView/operationDetailsTable/__snapshots__/index.test.js.snap

This snapshot file is very very huge and is basically useless. I would try to remove snapshot testing from this, to introduce some expect statements

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro

Q: are any of these changes possible with antd 3.x, i.e. before the actual upgrade?

anshgoyalevil · 2023-08-26T02:13:39Z

Q: are any of these changes possible with antd 3.x, i.e. before the actual upgrade?

Not really, because most of these changes include type updates, which are supplied by antd 4.x.
If we talk about migration from snapshot testing, that could be done before this PR, but I am planning of doing it in other PRs, as it would anyway not affect the antd upgrade, though some changes are needed to the failing tests.

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

codecov · 2023-08-27T19:29:07Z

Codecov Report

Patch coverage: 96.87% and project coverage change: -0.06% ⚠️

Comparison is base (0110ed0) 96.31% compared to head (936976a) 96.26%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1734      +/-   ##
==========================================
- Coverage   96.31%   96.26%   -0.06%     
==========================================
  Files         239      239              
  Lines        7492     7495       +3     
  Branches     1964     1964              
==========================================
- Hits         7216     7215       -1     
- Misses        276      280       +4

Files Changed	Coverage Δ
...aeger-ui/src/components/App/TraceIDSearchInput.tsx	`100.00% <ø> (ø)`
...ger-ui/src/components/Monitor/EmptyState/index.tsx	`92.85% <ø> (ø)`
...r-ui/src/components/SearchTracePage/FileLoader.tsx	`75.00% <0.00%> (-25.00%)`	⬇️
...r-ui/src/components/SearchTracePage/SearchForm.jsx	`92.15% <ø> (ø)`
...i/src/components/TracePage/TraceSpanView/index.tsx	`89.39% <ø> (ø)`
...kages/jaeger-ui/src/components/TracePage/index.tsx	`99.42% <ø> (ø)`
...kages/jaeger-ui/src/components/common/CopyIcon.tsx	`100.00% <ø> (ø)`
...es/jaeger-ui/src/components/common/UiFindInput.tsx	`100.00% <ø> (ø)`
packages/jaeger-ui/src/components/App/TopNav.tsx	`94.44% <100.00%> (ø)`
...i/src/components/DeepDependencies/Header/index.tsx	`100.00% <100.00%> (ø)`
... and 9 more

... and 2 files with indirect coverage changes

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

anshgoyalevil · 2023-08-27T19:31:58Z

@yurishkuro
Could you please verify the visual changes which are introduced with antd v4. Though, I have checked and changed every inch of the UI, but your view would be appreciated.

anshgoyalevil · 2023-08-27T19:49:32Z

There are some known significant visual changes in v4, which I haven't modified, like the Header Dropdown for About Jaeger is now replaced with three dots, and then the dropdown
Should I fix it too?

yurishkuro · 2023-08-27T22:53:49Z

There are some known significant visual changes in v4 ...

This is why I don't like this lock step upgrade that requires a huge PR. It would be easier if we could do changes component by component.

If you know the "known" changes, let's list then and decide one by one.

yurishkuro · 2023-08-28T03:37:43Z

Are you certain that there were no intermediate versions of antd where both old and new style components were available? Usually frameworks give a grace period for upgrades where old way is deprecated (and logs warnings) but is still working, precisely to allow for this kind of situation where a big bang upgrade is too large to do in a single change.

anshgoyalevil · 2023-08-28T03:50:44Z

Yes. Most probably, I checked that by manually installing the initial few versions of antd v4, ranging from 4.0.0-rc.0 to 4.12.3

Here's that quick analysis:

4.12.3 issue persists all over
4.2.0 issue persists all over
4.1.0 issue persists in the grid, but header is good
4.0.0 issue persists in the grid, but header is good
4.0.0-rc.0 issue persists in the grid, but header is good

The header thing looked easy to tackle so, I changed that too in the v4.23.13, as, a few other things were breaking in the v4.1.0

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil · 2023-08-28T04:20:27Z

@yurishkuro
I have framed the old vs. new table. Could you please check it out?

https://gist.github.com/anshgoyalevil/6081a14efbc9c7defa436a3a96263c0d

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro · 2023-08-28T04:58:52Z

4.12.3 issue persists all over

Do you mean compile errors?

If there are minimal errors in earlier versions, could we split this PR and merge in phases?

yurishkuro · 2023-08-28T04:59:52Z

cc @mmorel-35 I remember you mentioned incremental upgrades of some deps to have more narrow scope of changes, was it for antd or other deps?

anshgoyalevil · 2023-08-28T05:03:46Z

4.12.3 issue persists all over

Do you mean compile errors?

If there are minimal errors in earlier versions, could we split this PR and merge in phases?

Yes, we could do that too. There are some compile errors, as well as some UI changes, like Monitor page's grid view.

Would have to start all over, to determine the exact changes, to split into multiple PRs. Should I close this PR and create a narrow PR? As it would be somewhat difficult to go back in these changes and check.

yurishkuro · 2023-08-28T05:49:04Z

How big of a change would be needed to go to antd/v4.0.0 as the first step? How many components would not compile or would not work correctly?

anshgoyalevil · 2023-08-28T06:06:33Z

Can't exactly say that currently. Will update here asap.

anshgoyalevil · 2023-08-28T07:00:04Z

I just checked the whole scenario at v4.0.0, and here's an overview:

Monitor page's UI got messed up
Trace Page -> Trace Spans Graph Page and Trace Statistics Page have some UI data errors (due to dataIndex change) | Same with compare page's Cohort Table
Lookup by Trace ID Search Form is not working
Search Page's form is not working
Some Icons on Trace Page Header are not being rendered.
Lots of type errors (Same as in v4.24.13)

Which components are same as per the main branch? [1]

Header (TovNav)
Header (Dropdown)
Form (Selectors) on Trace Spans Table page

Can we narrow down the scope of this PR:

Yes, we can upgrade the antd to v4.0.0 and revert the changes mentioned in [1] above.
I wouldn't recommend that though, since these changes are just 5% of the whole PR, and removing or adding them won't affect much.

mmorel-35 · 2023-08-28T09:06:27Z

cc @mmorel-35 I remember you mentioned incremental upgrades of some deps to have more narrow scope of changes, was it for antd or other deps?

I'm not sure. But, concerning antd migration, the guide seems to have a long todo list. They even have a codemod cli to help on that task from I can read.
Depending on the dependency, it seems like the size of the increment will vary...

anshgoyalevil · 2023-08-31T14:01:10Z

@yurishkuro Any views upon this? #1734 (comment)

yurishkuro · 2023-08-31T14:46:22Z

It seems the basic prep they suggest is to move to the latest 3.x version and make sure that no deprecation warnings are showing. I assume they may have added new things in 3.x already while supporting the old behaviors. If that's the case then we can probably make some incremental changes before 4.x upgrade?

anshgoyalevil · 2023-08-31T17:16:39Z

So here's what I got so far:

Type Changes:
- React refs for AntD Input components need a typing update. (Only introduced in v4.x)
- The NumberInput component now guarantees to return number | null, so some custom type-checking code related to that can be removed. (Only introduced in v4.x)
- Typing changed around the Table component around row selection and sorting logic, requiring appropriate updates. In particular, sorting by multiple sortable columns now reports all columns that are active in sorting, so event handling code must also accept an array. (Only introduced in v4.x)
- The trace search form needs a workaround to function with redux-form, as AntD Forms no longer expose an onSubmit event that this library could hook into. For now, this is done by attaching the submit callback directly to the submit button instead as a click handler (which, somehow, still catches Enter keypresses on the search form). (Can be done before this PR, using antd/compatible library)
Since there is no deprecation warning, the component and styling changes are introduced in v4.x

What can we do before this PR?

Use antd/compatible library
Make some changes related to the grid.

Here's how I would be going:

PR for adding antd/compatible lib and making changes to grid
PR for upgrading antd to v4.0.0 (type fixes), so header won't need any change
Upgrade to latest version of antd 4.x with header changes.

## Which problem is this PR solving? - This PR is a part of the antd v3 to v4 process - Related to #1734 ## Description of the changes - This PR adds a ant-design/compatible package to make form work with the antd v4. Since antd v4 introduces some breaking changes with respect to redux, and redux-form. - The nested grids are fixed ## How was this change tested? - Manually as well as automated testing ## What's next? - The antd lib would need to be upgraded to v4.0.0 to fix type errors ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

## Which problem is this PR solving? - part of: #1734 - This PR is a part of upgrading antd to latest version of 4.x ## Description of the changes - Disabled the NavBar hover effect which was un-necessarily introduced - NavBar's height was increased internally. Fixed it to match previous height (48px) - The Monitor -> EmptyPage was using the offsets to align content to the center, but it is not required now, as it is a default behavior - `SorterResult` API is removed, so used lodash in place of that - `CompareFn` typedef is no longer required - Mocked the `window.matchMedia`, since some tests were using it - `dataIndex` now requires an array instead of nested string. Ex: `"abc.def.ghi"` -> `["abc", "def", "ghi"]` - Tests are updated according to these changes - Snapshots are updated according to internal changes in components ## How was this change tested? - Manually, and using automated testing ## What's next? - In the next PRs, the antd would be upgraded to 4.20.0 (or higher), to minimise the changes in a single PR. - New type errors would be introduced thereby, like NumberInput lookback changes, etc. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

## Which problem is this PR solving? - part of the upgrade process of ant-design - related to: #1734 ## Description of the changes - This PR fixes NavBar's dropdown "About Jaeger" which was changed due to some internal changes in antd v4.16.0 ## How was this change tested? - Manually, and automated testing ## What's Next? - antd would be upgraded to latest version of v4.x ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

## Which problem is this PR solving? - part of: #1734 - part of the ant-design dependency upgrade process ## Description of the changes - This PR changes the type from `Input` to `InputRef` as per antd typescript changes. - InputNumber now guarantees the type of input to be `number | null` which was previously `string | number | undefined` - Tests related to these change are updated accordingly. ## How was this change tested? - manually, and automated testing ## What's next? - This is the final PR for antd upgrade to version 4. ## Checklist - [x] I have read https://github.com/jaegertracing/jaeger/blob/master/CONTRIBUTING_GUIDELINES.md - [x] I have signed all commits - [x] I have added unit tests for the new functionality - [x] I have run lint and test steps successfully - for `jaeger`: `make lint test` - for `jaeger-ui`: `yarn lint` and `yarn test` --------- Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil · 2023-09-04T06:16:33Z

Completed in parts with #1748 #1750 #1751

anshgoyalevil added 5 commits August 25, 2023 15:09

Initial Upgrade

3239aa2

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

change ci

c601622

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

change ci

7474688

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

updates tests to mock window.matchMedia

054065e

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

mock window.matchMedia for other files also

db7b8aa

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil requested review from everett980 and rubenvp8510 as code owners August 25, 2023 11:26

Merge branch 'main' into bump2

973824c

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil commented Aug 25, 2023

View reviewed changes

anshgoyalevil marked this pull request as draft August 25, 2023 11:35

fix react-json-view-lite yarn.lock

9f1b482

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

yurishkuro reviewed Aug 25, 2023

View reviewed changes

anshgoyalevil added 4 commits August 26, 2023 09:16

add antd compatible and fix TopNav

6b41d27

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

change TopNav.tsx styles

56aefac

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

upgrade antd to v4.24.13 and fix monitor page

cd6986f

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

fix tests for operationDetailsTable

941594f

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil changed the title ~~[WIP] Upgrade antd to 4.24.8~~ [WIP] Upgrade antd to 4.24.13 Aug 27, 2023

anshgoyalevil added 6 commits August 27, 2023 19:01

Update tests and snapshots

82c3202

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

fix QualityMetrics Tests

2b1d8cf

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

use updated dataindex def

f9f4a2a

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

update snapshorts for CohortTable tests

9571703

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

fix TracePage and related tests

ed31b01

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

window.matchMedia refactor

c6eb27a

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil marked this pull request as ready for review August 27, 2023 19:18

anshgoyalevil changed the title ~~[WIP] Upgrade antd to 4.24.13~~ Upgrade antd to 4.24.13 Aug 27, 2023

anshgoyalevil added 3 commits August 28, 2023 09:22

fix header

51b8454

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

fix fmt

3189763

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

fix header colors

2087400

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

anshgoyalevil added 2 commits August 28, 2023 10:16

fix yarn.lock conflicts

21be849

Signed-off-by: Ansh Goyal <anshgoyal1704@gmail.com>

Merge branch 'main' into bump2

936976a

anshgoyalevil mentioned this pull request Aug 31, 2023

Add antd/compatible package for form and fix grids #1746

Merged

4 tasks

anshgoyalevil marked this pull request as draft September 1, 2023 09:21

anshgoyalevil mentioned this pull request Sep 1, 2023

Upgrade antd to v4.15.6 #1748

Merged

4 tasks

anshgoyalevil mentioned this pull request Sep 3, 2023

Upgrade antd to v4.16.0 from v4.15.6 #1750

Merged

4 tasks

anshgoyalevil mentioned this pull request Sep 4, 2023

Upgrade antd to v4.24.13 from v4.16.0 #1751

Merged

4 tasks

anshgoyalevil closed this Sep 4, 2023

anshgoyalevil deleted the bump2 branch September 4, 2023 12:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade antd to 4.24.13 #1734

Upgrade antd to 4.24.13 #1734

anshgoyalevil commented Aug 25, 2023 •

edited

Loading

anshgoyalevil Aug 25, 2023 •

edited

Loading

yurishkuro left a comment

anshgoyalevil commented Aug 26, 2023

codecov bot commented Aug 27, 2023 •

edited

Loading

anshgoyalevil commented Aug 27, 2023 •

edited

Loading

anshgoyalevil commented Aug 27, 2023

yurishkuro commented Aug 27, 2023

yurishkuro commented Aug 28, 2023 •

edited

Loading

anshgoyalevil commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023

yurishkuro commented Aug 28, 2023

yurishkuro commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023 •

edited

Loading

yurishkuro commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023 •

edited

Loading

mmorel-35 commented Aug 28, 2023

anshgoyalevil commented Aug 31, 2023

yurishkuro commented Aug 31, 2023

anshgoyalevil commented Aug 31, 2023 •

edited

Loading

anshgoyalevil commented Sep 4, 2023

Upgrade antd to 4.24.13 #1734

Upgrade antd to 4.24.13 #1734

Conversation

anshgoyalevil commented Aug 25, 2023 • edited Loading

Which problem is this PR solving?

Description of the changes

Checklist

anshgoyalevil Aug 25, 2023 • edited Loading

Choose a reason for hiding this comment

yurishkuro left a comment

Choose a reason for hiding this comment

anshgoyalevil commented Aug 26, 2023

codecov bot commented Aug 27, 2023 • edited Loading

Codecov Report

anshgoyalevil commented Aug 27, 2023 • edited Loading

anshgoyalevil commented Aug 27, 2023

yurishkuro commented Aug 27, 2023

yurishkuro commented Aug 28, 2023 • edited Loading

anshgoyalevil commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023

yurishkuro commented Aug 28, 2023

yurishkuro commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023 • edited Loading

yurishkuro commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023

anshgoyalevil commented Aug 28, 2023 • edited Loading

Which components are same as per the main branch? [1]

Can we narrow down the scope of this PR:

mmorel-35 commented Aug 28, 2023

anshgoyalevil commented Aug 31, 2023

yurishkuro commented Aug 31, 2023

anshgoyalevil commented Aug 31, 2023 • edited Loading

anshgoyalevil commented Sep 4, 2023

anshgoyalevil commented Aug 25, 2023 •

edited

Loading

anshgoyalevil Aug 25, 2023 •

edited

Loading

codecov bot commented Aug 27, 2023 •

edited

Loading

anshgoyalevil commented Aug 27, 2023 •

edited

Loading

yurishkuro commented Aug 28, 2023 •

edited

Loading

anshgoyalevil commented Aug 28, 2023 •

edited

Loading

anshgoyalevil commented Aug 28, 2023 •

edited

Loading

anshgoyalevil commented Aug 31, 2023 •

edited

Loading