Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hari45678
Copy link
Contributor

Which problem is this PR solving?

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

Signed-off-by: Hariom Gupta <guptahariom03082003@gmail.com>
Signed-off-by: Hariom Gupta <guptahariom03082003@gmail.com>
Signed-off-by: Hariom Gupta <guptahariom03082003@gmail.com>
@hari45678 hari45678 requested a review from a team as a code owner January 17, 2025 07:52
@hari45678 hari45678 requested review from mahadzaryab1 and removed request for a team January 17, 2025 07:52
Signed-off-by: Hariom Gupta <guptahariom03082003@gmail.com>
Copy link

codecov bot commented Jan 17, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.61%. Comparing base (1178b1c) to head (ec37b0b).

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 <guptahariom03082003@gmail.com>
<Popover placement="bottomLeft" open={isInvalid} {...validationResult}>
<Input
className={cx({
'duration-is-invalid': isInvalid,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this appears to be a generic form field component, why is style called duration-*?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's a generic component but is currently used for duration fields (minDuration and maxDuration) in the Search Sidebar form. I will rename that to a more generic classname, if there arise any use case for this component in other part of the code.

@@ -0,0 +1,46 @@
// Copyright (c) 2017 Uber Technologies, Inc.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this file copied from somewhere? If it's brand new the header should be

// Copyright (c) 2025 The Jaeger Authors.
// SPDX-License-Identifier: Apache-2.0

(same for the test file)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not copied. I created this custom component

@@ -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';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no replacement for this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 <guptahariom03082003@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Deps] Remove dependency on redux-form
2 participants