Don't reset catalog on first render.

[absoludity]

Signed-off-by: Michael Nelson minelson@vmware.com

Description of the change

This change is to fix an issue that I've identified as part of #4694, where we have a tension between global redux state and local state.

The issue is that:

• We currently use global redux state, so if you navigate to the catalog and then navigate away, the state of the catalog is retained (items, last token etc.)
• When you navigate back to the catalog, an initial request goes off to fetch the available package summaries, followed by another effect to reset the state

Two possible approaches:

1. Update the reset effect so that it is only run when a dependency changes (not on the initial render) - this is what's done here, but it means we'd also need to reset the state separately when the namespace is changed outside of the component (may be ok). The downside is that it moves us toward global state, where as I think we want to move away from it.
2. Ensure that the state is reset when the component is unmounted. This would be more inline with moving towards non-global state, but doesn't quite work either OOTB, because requests are in flight, so it resets the state as you move away from the component, but then a response comes back from the API and is handled updating the redux state after the reset. So to avoid this, I'd update the reducer so that it doesn't update the state if isFetching is false (makes sense).

So I'm leaning towards (2). WDYT? I've updated the PR to do 2. It now resets the state when the component is unmounted and does not update the state when subsequent available package summary responses are received (without being requested, ie. after a reset).

Tested with #4694 and it works perfectly, with paginated results never getting confused.

Benefits

Ensures that (a) we don't send off requests for available package summaries with differing (pagination) state, and (b) we are one step closer to converting the component away from global redux state.

Possible drawbacks

Applicable issues

• ref Implement pagination in the aggregated core APIs #3399

Additional information

[castelblanque]

+1 to the second option.

I don't see global state needed for this use case. If requests are sent when component is mounted, and data is going to be reset when unmounted: why do we need to set that data in the global redux state?

[absoludity]

+1 to the second option.

Great, let me do the second option :)

I don't see global state needed for this use case. If requests are sent when component is mounted, and data is going to be reset when unmounted: why do we need to set that data in the global redux state?

We don't if we head in this direction (which I think we want to). Currently the state is maintained and we could improve that so it doesn't re-request when it already has the data, but given that we want to move away from global state, we'll do option 1 above which doesn't tie us to the global state and we can more easily remove it in a later PR.

[antgamdia]

Great! +1 to this option (2). Apart from some really common data (like ns/cluster/etc... which we are already encoding as part of the URL), I don't think we need a global state. And, when it comes to pagination, it should be scoped just to the component, and nothing else.

Thanks for implementing!

In your IRL test, have you checked if you are triggering just 1 request per page? I mean, the problem I faced when implementing the pagination was the same page being requested multiple times, so the UI had to deal with duplicates.
Like page(0), page(0), page(1) or page(0), page(1), page(0)

[antgamdia]

Do you know if the cleanup function has necessarily to be cleanup() ? In other places we just have return () => {//whatever}; ?
I mean, sth like:

Suggested change

	return function cleanup() {
	return () => {

[absoludity]

It doesn't have to be a named function, it works fine as an anonymous function, but as in the React docs, I prefer a named one here just so it's obvious to anyone reading the code what the purpose of the returned function is.

[absoludity]

In your IRL test, have you checked if you are triggering just 1 request per page? I mean, the problem I faced when implementing the pagination was the same page being requested multiple times, so the UI had to deal with duplicates.
Like page(0), page(0), page(1) or page(0), page(1), page(0)

Yes - that's exactly the problem it's fixing. The issue was that all effects are run initially when the page is rendered, so the effect for requesting the first page would run, updating the state, then the effect resetting the state would run, triggering another request requesting the first page. The response then comes back for the initial request, updating the state, then the second, overwriting the state etc. The effect is worse when paginating multiple plugins and it would get into cycles.

With this change, you can follow the redux actions and see that it no longer resets the state on the initial render, but will reset when anything changes or when the component is unmounted, which is exactly what was needed.