CUMULUS-3811 Refactor Data Management Class Components

[wisdomaj]

Summary:
Refactor data management class components to functional components

Addresses CUMULUS-3811 Refactor Data Management Class Components

Changes

• Converting class components to functional components
• Use React and Redux state hooks to react to state changes instead of parameterized constructors

PR Checklist

• Update CHANGELOG
• Unit tests
• Adhoc testing
• Integration tests

[Nnaga1]

Hey Austin! Great work! some comments I had 👀

[Nnaga1]

I notice you remove this:

  queryParams={filteredQueryParams}
                    {...props}

and do:
const queryParams = Object.fromEntries(new URLSearchParams(location.search)); and the filtering elsewhere, is there a reasoning for this?: calling it in the overview vs getting it passed down, you make this change in several other spots to, just wondering

[wisdomaj]

Whether to pass query params as a prop or use a hook was something I was debating myself and wondering if I went to far.

Since nearly all props for all of the components I refactored were deriving their values from the global redux state anyways, I removed those props in favor of using hooks.

ie.

export default withRouter(
  withQueryParams()(
      connect((state) => ({
        collections: state.collections,
        config: state.config,
        granules: state.granules,
        workflowOptions: workflowOptionNames(state),
        providers: state.providers,
      }))(GranulesOverview)
    )
);

For many components the query params would be the only prop left. I thought we could simply the component further by removing query params too but this wasn't really necessary. There are valid arguments to be made for still keeping some props, notably the query params.

If we use a hook for query params, one improvment would be to create a shared hook that can be reused in every component (currently I'm creating a new hook in each component for query params).

We could just revert those changes and pass query params as a prop instead too. I don't have strong feelings either way.

[wisdomaj]

Edit: Since withRouter is removed in react router v6, we will need to use hooks for reacting to url changes going forward anyways. I think the question is whether we should:

1. Use a shared hook for query params for all components that need it
2. Use a shared hook to get query params in a parent component and then pass those query params down to child components as props

[Nnaga1]

thanks for the detailed explanation, I did notice that pretty much there are no props being used, which is a result of the work you are doing, and if queryParams is the only prop we're using and passing, then maybe it would make more sense in the actual component its being used in rather than being passed into it,

if you noticed no/decreased performance changes from this I would support it, especially, since, as you mentioned, moving forward we're gonna be moving away from the router (which I was trying to do with Allan way earlier this year but found that it would be difficult since it was super ingrained), has then been the case? I'll leave this up to you/other dashboard people since it could go either way

[Nnaga1]

one other question I thought of while typing that ^ are the queryParams pretty much only passed from parent -> child in the overviews? i.e. does the parent pass them down to multiple children or just one pretty much?

[wisdomaj]

I've only noticed query params passed down from each index.js file to child components like GranulesOverview (I've noticed one layer of query param props being passed). However, it's possible in other parts of the dashboard there are heavily nested routes and multiple layers of query params being passed down to child components.

[Nnaga1]

can you explain why the Router isn't needed anymore? I'm a little confused on that

[wisdomaj]

withRouter wraps a component so it re-renders when the URL (location) changes and it injects route properties like history, location, and match. Since this component is now using the useLocation hook to react to url changes withRouter is no longer needed.

Sidenote: I'm seeing that withRouter is removed from react router v6 so we want to upgrade to v6 in the future we will need to remove withRouter and use hooks instead anyways.

[acyu]

I agree with this change, in the React router 6 upgrade ticket Juanisa made the same change as well.

[Nnaga1]

yea there was an effort earlier this year to try to remove the reactRouter/upgrade it but it came up short because we found it was too ingrained and would take a ton of effort we couldn't put in back then, this is a good change moving away from it but I wasn't exactly sure how it worked, but now seeing how the useLocation hook pretty much replaces it functionally makes sense 👍

[Nnaga1]

same question as with Granules with the queryParams

[acyu]

This completely removed url-helper from the component. historyPushWithQueryParams is still used in other components. Do we no longer need historyPushWithQueryParams? As far as I know historyPushWithQueryParams is for persisting parameters by using history.push. Or it's just we don't need it for this particular component?
This apply to navigateBack function in line 30

[wisdomaj]

I had just removed it in this particular component because this navigateBack function was defined but not actually used in the component:

navigateBack() {
  historyPushWithQueryParams('/executions');
}

[acyu]

Ok, I may need another set of eye on this. The new changes had move the component away from the router. or at least, the search parameters should already existed and available to this component without the component has to explicitly parse the search parameters. I am not sure I agree with this approach

[acyu]

Same here, Providers is already getting queryParams, by removing the filteredQueryParams we now has an unused argument.

[acyu]

Can you elaborate a bit, I am not sure why we are doing this checking. The previous logic checks for value change where here checks for whether value is there. I am assuming the logic change is because of the useEffect dependency, but why do if ruleName checking

[wisdomaj]

This should be functionally equivalent.

useEffect(() => {
  if (ruleName) {
    load(ruleName);
  }
}, [ruleName, load]);

The useEffect hook will rerun whenever the dependencies change. The ruleName and load in this useEffect hook are the dependencies so when ruleName changes, useEffect will run again and load the new ruleName.

[acyu]

I agree with this change, in the React router 6 upgrade ticket Juanisa made the same change as well.