[Deps]: Remove dependency on redux-form #2593

hari45678 · 2025-01-17T07:52:19Z

Which problem is this PR solving?

resolves [Deps] Remove dependency on redux-form #2556

Description of the changes

This PR removes the redux-form dependency completely.
The state is now managed with built-in react state management.
A separate ValidatedFormField component is created to handle the Validated minDuration and maxDuration fields that require validation and Popover to be displayed on failed validations.
A middle ware was being used to fetch operations based on the selected service. That middleware was earlier being triggered on selecting a redux-form field, but would now trigger on selecting a new service from Service Selector.

How was this change tested?

npm run test
npm run start (compared the jaeger-ui 1.65.0 with this PR changes)

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: Hariom Gupta <[email protected]>

codecov · 2025-01-17T08:02:11Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (1178b1c) to head (26ddb63).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2593      +/-   ##
==========================================
+ Coverage   96.58%   96.61%   +0.03%     
==========================================
  Files         255      255              
  Lines        7732     7744      +12     
  Branches     2009     1998      -11     
==========================================
+ Hits         7468     7482      +14     
+ Misses        264      262       -2

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

Signed-off-by: Hariom Gupta <[email protected]>

packages/jaeger-ui/src/utils/ValidatedFormField.tsx

yurishkuro · 2025-01-18T00:59:57Z

packages/jaeger-ui/src/constants/search-form.tsx

@@ -16,4 +16,4 @@ export const DEFAULT_OPERATION = 'all';
 export const DEFAULT_LOOKBACK = '1h';
 export const DEFAULT_LIMIT = 20;

-export const FORM_CHANGE_ACTION_TYPE = '@@redux-form/CHANGE';
+export const FORM_CHANGE_ACTION_TYPE = '@@redux/searchSideBar/CHANGE_SERVICE';


FORM_CHANGE is a very generic name while the event seems to be a change in a single specific dropdown

yurishkuro · 2025-01-18T01:00:47Z

packages/jaeger-ui/src/components/SearchTracePage/SearchForm.jsx

@@ -673,23 +719,19 @@ export function mapStateToProps(state) {
      traceIDs: traceIDs || null,
    },
    searchMaxLookback: _get(state, 'config.search.maxLookback'),
-    selectedService: searchSideBarFormSelector(state, 'service'),
-    selectedLookback: searchSideBarFormSelector(state, 'lookback'),


no replacement for this one?

redux-form maps the redux state to props and those props are then used in the component. These values were being populated after a user selects a service or lookback and are then used to conditionally show the startDate startDateTime endDate endDateTime fields, and also to trigger the middleware to load updated operations for the selectedService.
This whole state is now being selected from fromData state, so there's no need for a replacement for this

Signed-off-by: Hariom Gupta <[email protected]>

packages/jaeger-ui/src/utils/ValidatedFormField.test.js

yurishkuro · 2025-01-19T17:53:05Z

packages/jaeger-ui/src/components/SearchTracePage/SearchForm.jsx

-    } = this.props;
+    const { invalid, searchMaxLookback, services, submitting: disabled } = this.props;
+    const { formData } = this.state;
+    const { service: selectedService, lookback: selectedLookback } = formData;


I noticed there is no selectedOperation. When reloading the Search screen the service remains, but the operation is reset to all. Could we retain operation as well? OK to create a separate issue.

yurishkuro · 2025-01-19T18:06:01Z

packages/jaeger-ui/src/constants/search-form.tsx

@@ -16,4 +16,4 @@ export const DEFAULT_OPERATION = 'all';
 export const DEFAULT_LOOKBACK = '1h';
 export const DEFAULT_LIMIT = 20;

-export const FORM_CHANGE_ACTION_TYPE = '@@redux-form/CHANGE';
+export const SEARCH_SIDEBAR_CHANGE_SERVICE_ACTION_TYPE = '@@redux/searchSideBar/CHANGE_SERVICE';


given the context of search-form package I think CHANGE_SERVICE_ACTION_TYPE would've been sufficient, but ok.

yurishkuro · 2025-01-19T18:11:03Z

packages/jaeger-ui/src/utils/ValidatedFormField.tsx

+  const isInvalid = blur && Boolean(validationResult);
+
+  return (
+    <Popover placement="bottomLeft" open={isInvalid} {...validationResult}>


one thing I noticed that if I put an invalid value into duration, like 100kg, I get this permanent tooltip popup, but the Find Traces button is still enabled, resulting in a server error if clicked. It would make sense to disable the button (can be a separate issue)

yurishkuro · 2025-01-19T18:13:14Z

packages/jaeger-ui/src/components/SearchTracePage/SearchForm.jsx

-      services,
-      submitting: disabled,
-    } = this.props;
+    const { invalid, searchMaxLookback, services, submitting: disabled } = this.props;


Personally I think it should always be called submitting, to reflect the state of the form. disabled is just a property of individual fields.

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro · 2025-01-19T18:16:52Z

Thanks! Feel free to address comments in the follow up PRs (but not a high priority)

hari45678 added 3 commits January 17, 2025 13:04

remove redux-form completely

c16e464

Signed-off-by: Hariom Gupta <[email protected]>

make prettier

d376629

Signed-off-by: Hariom Gupta <[email protected]>

remove redux-form from lockfile

621feea

Signed-off-by: Hariom Gupta <[email protected]>

hari45678 requested a review from a team as a code owner January 17, 2025 07:52

hari45678 requested review from mahadzaryab1 and removed request for a team January 17, 2025 07:52

fix snapshot unexpected change

3ade5b4

Signed-off-by: Hariom Gupta <[email protected]>

add more tests

3621a1f

Signed-off-by: Hariom Gupta <[email protected]>

yurishkuro reviewed Jan 18, 2025

View reviewed changes

hari45678 mentioned this pull request Jan 18, 2025

[CI] Fix cache resolution and syntax in check binary size workflow #2591

Merged

4 tasks

hari45678 added 3 commits January 18, 2025 10:32

nit: change some variable names

ec37b0b

Signed-off-by: Hariom Gupta <[email protected]>

change license headers on new files

d42edca

Signed-off-by: Hariom Gupta <[email protected]>

add more to that header

3bbd294

Signed-off-by: Hariom Gupta <[email protected]>

yurishkuro reviewed Jan 19, 2025

View reviewed changes

packages/jaeger-ui/src/utils/ValidatedFormField.test.js Outdated Show resolved Hide resolved

yurishkuro reviewed Jan 19, 2025

View reviewed changes

Remove redundant license text

26ddb63

Signed-off-by: Yuri Shkuro <[email protected]>

yurishkuro approved these changes Jan 19, 2025

View reviewed changes

yurishkuro added the changelog:bugfix-or-minor-feature 🐞 Bug fixes, Minor Improvements label Jan 19, 2025

yurishkuro enabled auto-merge (squash) January 19, 2025 18:15

yurishkuro merged commit 657cd00 into jaegertracing:main Jan 19, 2025
9 of 10 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Deps]: Remove dependency on redux-form #2593

[Deps]: Remove dependency on redux-form #2593

hari45678 commented Jan 17, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading

yurishkuro Jan 18, 2025

yurishkuro Jan 18, 2025

hari45678 Jan 18, 2025

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yurishkuro Jan 19, 2025

yurishkuro commented Jan 19, 2025

[Deps]: Remove dependency on redux-form #2593

[Deps]: Remove dependency on redux-form #2593

Conversation

hari45678 commented Jan 17, 2025

Which problem is this PR solving?

Description of the changes

How was this change tested?

Checklist

codecov bot commented Jan 17, 2025 • edited Loading

Codecov Report

yurishkuro Jan 18, 2025

Choose a reason for hiding this comment

yurishkuro Jan 18, 2025

Choose a reason for hiding this comment

hari45678 Jan 18, 2025

Choose a reason for hiding this comment

yurishkuro Jan 19, 2025

Choose a reason for hiding this comment

yurishkuro Jan 19, 2025

Choose a reason for hiding this comment

yurishkuro Jan 19, 2025

Choose a reason for hiding this comment

yurishkuro Jan 19, 2025

Choose a reason for hiding this comment

yurishkuro commented Jan 19, 2025

codecov bot commented Jan 17, 2025 •

edited

Loading