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.

Sign up for GitHub

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

Jump to bottom

Upgrade to react-router-6 #6187

Closed
absoludity opened this issue Apr 14, 2023 · 2 comments · Fixed by #6504
Closed

Upgrade to react-router-6 #6187

absoludity opened this issue Apr 14, 2023 · 2 comments · Fixed by #6504
Assignees
@absoludity
Labels
component/ui Issue related to kubeapps UI kind/enhancement An issue that reports an enhancement for an implemented feature
Milestone
Technical debt

Comments

@absoludity
Copy link
Contributor

Summary
For a while now we've been avoiding the upgrade to react-router 6 (eg #6153), as it will contain a significant chunk of work.

There are tools to help with the migration (so that you can update the code piecemeal (see upgrading from v5), but I'm sure there will also be unknowns along the way.

I've not yet found any clear info about when react router react-router v5 will cease support.
@github-project-automation github-project-automation bot moved this to 🗂 Backlog in Kubeapps Apr 14, 2023
@ppbaena ppbaena added the kind/enhancement An issue that reports an enhancement for an implemented feature label Apr 17, 2023
@ppbaena ppbaena added this to the Technical debt milestone May 8, 2023
@ppbaena ppbaena moved this from 🗂 Backlog to 🗒 Todo in Kubeapps May 8, 2023
@ppbaena ppbaena added the component/ui Issue related to kubeapps UI label May 8, 2023
@absoludity absoludity mentioned this issue Jul 12, 2023
Merged
@absoludity absoludity moved this from 🗒 Todo to 🏗 In Progress in Kubeapps Jul 12, 2023
@absoludity
Copy link
Contributor Author

I've done step 1 here, which is refactoring custom routes.

@absoludity
Copy link
Contributor Author

I've done step 1 here, which is refactoring custom routes.

Or not... turns out we'll need to convert the Operator container+component classes to function components to be able to use the new route system... so today's task is to do that prequel PR then be able to update the PR from yesterday to use it :/

@absoludity absoludity mentioned this issue Jul 13, 2023
Merged
absoludity added a commit that referenced this issue Jul 13, 2023
@absoludity
Switch OperatorNew to react-router hooks and remove OperatorNew conta…
6f55e10 
…iner (#6457)

### Description of the change

Part 1 in a series of prequel PRs for #6187 upgrading react-router, this
PR switches the OperatorNew component to use the react-router hooks and
removes the container.

### Benefits

1 step closer to #6187

### Possible drawbacks

Let CI decide.

### Applicable issues

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Jul 13, 2023
Merged
absoludity added a commit that referenced this issue Jul 14, 2023
@absoludity
Switch 2 more operator components to use hooks and remove containers. (…
c08f865 
…#6458)

### Description of the change

Part 2 in a series of prequel PRs for #6187 upgrading react-router, this
PR switches the OperatorInstanceForm and OperatorList components to use
the react-router hooks and removes the containers.

### Benefits

1 step closer to #6187

### Possible drawbacks

Let CI decide.

### Applicable issues

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Jul 14, 2023
Merged
absoludity added a commit that referenced this issue Jul 14, 2023
@absoludity
Covert OperatorView, OperatorInstance and OperatorInstanceForm. (#6465)
449800f 
### Description of the change

Part 3 in a series of prequel PRs for #6187 upgrading react-router, this
PR switches the OperatorInstance, OperatorInstanceUpdateForm and
OperatorView components to use the react-router hooks and removes the
containers.

### Benefits

1 step closer to #6187

### Possible drawbacks

Let CI decide.

### Applicable issues

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Jul 17, 2023
Merged
absoludity added a commit that referenced this issue Jul 17, 2023
@absoludity
Convert the LoginForm to react-router. (#6478)
8c2bad3 
### Description of the change

Continuing the work towards #6187, this PR switches the LoginForm to use
react-router and hooks.

### Benefits

One step closer to react-router upgrade.

### Applicable issues

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
absoludity added a commit that referenced this issue Jul 17, 2023
@absoludity
Prequel to upgrading to react-router 6 (#6453)
8c98273 
### Description of the change

Updates our routing with some preparation for the react-router upgrade.
This involved:

- Re-writing the class-based PrivateRoute component to use
function/hooks,
- Refactoring the use of `Route` out of the PrivateRoute component and
into the routes directly, as these need to be top-level for react-router
6
- Renaming PrivateRoute to RequireAuthentication (as it no longer
generates a route).
- Updating to simplify tests using react testing library rather than
enzyme, including the renderWithProviders util (as per recommended docs)

See https://reactrouter.com/en/main/upgrading/v5

### Benefits

One step closer to updating to react-router 6.
Starts transition from enzyme to react testinglibrary.

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Jul 21, 2023
Merged
@absoludity absoludity mentioned this issue Jul 31, 2023
Merged
absoludity added a commit that referenced this issue Jul 31, 2023
@absoludity
Remove custom ErrorBoundary container and component. (#6542)
89633cc 
### Description of the change

As part of #6187, upgrading react-router to v6, we also need to remove
the remaining containers so we can remove connected-react-router.

### Benefits

Enables #6187 to continue forward.

### Possible drawbacks


### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6187

### Additional information

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Jul 31, 2023
Merged
absoludity added a commit that referenced this issue Jul 31, 2023
@absoludity
Removes ApplicationStatusContainer and AccessURLTableContainer (#6544)
23dac7b 
### Description of the change

Removes two more uses of containers from the code base (last two - yay).

### Benefits

Complete move to react hooks and stop maintaining mixture of both.

### Possible drawbacks

### Applicable issues

- ref #6187 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Aug 3, 2023
Merged
absoludity added a commit that referenced this issue Aug 3, 2023
@absoludity
Ensure token auth can be configured while oauthLoginURI is defined. (#…
d199023 
…6557)

### Description of the change

When switching from a container to a component with hooks, I missed that
the prop was passed from the container as:

```
   oauthLoginURI: authProxyEnabled ? oauthLoginURI : "",
```

but with the component the oauthLoginURI was always defined.

### Benefits

Can configure token auth again

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6187

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

Signed-off-by: Michael Nelson <[email protected]>
@github-project-automation github-project-automation bot moved this from 🏗 In Progress to ✅ Done in Kubeapps Aug 3, 2023
absoludity added a commit that referenced this issue Aug 3, 2023
@absoludity
Actual react-router-dom upgrade (#6504)
7b4a984 
### Description of the change

After the prequal work, this PR does the actual upgrade of
react-router-dom, including updating all the use of `Route`, as well as
removing connected-react-router and the remaining container components.
Mostly following from their docs for the upgrade at:
https://reactrouter.com/en/main/upgrading/v5#upgrade-to-react-router-v6
with some custom changes required by our specific test infrastructure.

### Benefits

Finally upgrade react-router and upgrade to hook-based components
(something that has been long-running tech-debt slowing us down).

### Possible drawbacks

There could be some fall-out of small UX issues that weren't caught in
CI. I know of one that I'll fix straight away (two requests sent off for
available packages when viewing the catalog - one for all apps in all
namespaces).

### Applicable issues

- fixes #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Aug 4, 2023
Merged
absoludity added a commit that referenced this issue Aug 6, 2023
@absoludity
Ensure only single request for installed apps. (#6568)
41e940a 
### Description of the change

Fixes an issue I'd noticed while upgrading react-router, in that
sometimes the installed apps for the namespace would display, then
seconds later, be updated with installed apps across all namespaces.

### Benefits

No longer have this confusing UI behaviour.

### Possible drawbacks

None.

### Applicable issues

- ref #6187

---------

Signed-off-by: Michael Nelson <[email protected]>
@absoludity absoludity mentioned this issue Aug 7, 2023
Merged
absoludity added a commit that referenced this issue Aug 7, 2023
@absoludity
Small changes from review on #6504 (#6571)
3891804 
### Description of the change

Addresses a couple of small points from the review on #6504 .

Turns out it didn't need any extra style - looks identical with the
label element (verified in the debugger that it does have the label
element etc.):
<img width="325" alt="Screenshot 2023-08-07 at 11 21 09 am"
src="https://github.com/vmware-tanzu/kubeapps/assets/497518/25fc16b7-168c-4f21-8f79-f6227237503a">
<img width="325" alt="Screenshot 2023-08-07 at 11 22 29 am"
src="https://github.com/vmware-tanzu/kubeapps/assets/497518/bcf3dbd1-4033-42bc-a60c-6d44600b4e8f">


### Benefits

Removed unnecessary test-ids and more accessible UX.

### Possible drawbacks

<!-- Describe any known limitations with your change -->

### Applicable issues

<!-- Enter any applicable Issues here (You can reference an issue using
#) -->

- ref #6187 

### Additional information

<!-- If there's anything else that's important and relevant to your pull
request, mention that information here.-->

---------

Signed-off-by: Michael Nelson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@absoludity absoludity

Labels
component/ui Issue related to kubeapps UI kind/enhancement An issue that reports an enhancement for an implemented feature
Projects
Kubeapps
Archived in project
Milestone
Technical debt
Development

Successfully merging a pull request may close this issue.

Actual react-router-dom upgrade vmware-tanzu/kubeapps
3 participants
@absoludity @ppbaena and others