Actual react-router-dom upgrade #6504

absoludity · 2023-07-21T05:51:16Z

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 Upgrade to react-router-6 #6187

netlify · 2023-07-21T05:51:23Z

✅ Deploy Preview for kubeapps-dev canceled.

Name	Link
🔨 Latest commit	`ea6ebde`
🔍 Latest deploy log	https://app.netlify.com/sites/kubeapps-dev/deploys/64cb5b44b72fcd00085607bf

absoludity · 2023-07-25T08:07:20Z

Painful day of debugging why updated tests using react-router-dom were now failing with what, after some painful investigation and pinpointing, seemed to be a clarity design system issue where browser-specific calls were being made (ie. that fail in nodejs and cause the test runner to barf). As always, solution was pretty simple (polyfill in tests and jest mock for the other occurrence), so that I can continue easily enough now. Though 135 tests left to update.

absoludity · 2023-07-28T05:36:31Z

So all the test code has now been updated for react-router v6 - great, but although the unit tests pass, the actual code fails because connected-react-router never updated to support v6 (and in reality, why should it - it's supporting the old non-hook way of maintaining state).

The end result is we need to remove it, which is only 4 containers left using mapStateToProps etc.

absoludity · 2023-08-02T06:14:32Z

So as well as removing/converting the last containers to proper components, removing the connected-react-router required adding our own custom hook (see hooks/push) so that we can update the browser history using the new navigate from react-router v6 while still dispatching a location changed action (as connected-react-router used to do) when the location is changed, so our reducers still have something to act on when it changes. Still need to verify it works and debug if not (once merge conflicts are resolved).

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

absoludity · 2023-08-03T08:39:50Z

FINALLY!! :P (there's still at least one issue, but I'll fix that separately ... just that when viewing the installed apps, two requests are sent off, one for all namespaces, so the view gets updated with apps from all namespaces after initially rendering).

absoludity · 2023-08-03T08:41:00Z

@antgamdia It won't be fun, so no problem if you're not keen, but feel free to take a look if you're interested. I'll plan to land it tomorrow with any recommendations :)

antgamdia

Sorry I didn't get to finish the review in time! Big +1 and thanks for such an effort in refactoring the whole UI "backstage". It's great to finally be able to move to the most recent react-router version!

antgamdia · 2023-08-04T07:57:55Z

dashboard/src/components/Header/ContextSelector.tsx

@@ -165,6 +166,7 @@ function ContextSelector() {
                className="clr-page-size-select"
                onChange={selectNamespace}
                value={namespace}
+                data-testid="select-namespace"


Is this field really intended?

Yeah, this was one that I really struggled to find a way to reference it by role or label in our tests, without a test-id (I didn't want to restructure the markup as part of the change). I just took another look and found I can add an aria-label attribute to the select for cluster and namespace, which is better than adding a test-id (since it'll actually function with a correct label text, accessibility-wise). We could also update the span elements to be labels but I didn't want to update the markup and, it seems, was being lazy, then I thought you're going to review this too and it's what you'd do, so I've done that instead... good reason for having reviewers. Pushing to a different PR.

Thanks for the explanation and follow-up PR. Happy to be back again!

antgamdia · 2023-08-04T08:02:28Z

dashboard/src/components/OperatorNew/OperatorNew.tsx

@@ -261,7 +263,7 @@ export default function OperatorNew() {
                </div>
              </div>
              <div className="clr-form-control">
-                <CdsButton type="submit" disabled={disableInstall}>
+                <CdsButton type="submit" aria-disabled={disableInstall}>


Why this change? From what I can see in a quick search, it seems both aria-disabled and disabled actually disable the input... however, the aria- one does not prevent the element to be announced in a screen reader. Is this the motivation of the change? If so, perhaps we should extend it to the rest of the inputs, no?

Nope, I'm guessing I was trying to get it to appear in the screen.getByRole() for a react testing library test. Removing and seeing if tests pass. They do - updated.

antgamdia · 2023-08-04T08:05:07Z

dashboard/src/hooks/push.tsx

@@ -0,0 +1,45 @@
+// Copyright 2018-2023 the Kubeapps contributors.


Technically this file is new, it should be just 2023 😅

Woops, yes, I usually just copy-n-paste an existing header. Seems I fixed this while removing react-router dependencies.

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

### 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  ### Applicable issues  - ref #6187 ### Additional information  --------- Signed-off-by: Michael Nelson <minelson@vmware.com>

vmwclabot added the cla-not-required label Jul 21, 2023

absoludity changed the title ~~WIP: working through failig tests... only 165 to go :/~~ Actual react-router-dom upgrade Jul 25, 2023

absoludity force-pushed the react-router-upgrade-2 branch from 6a00ee3 to 92022f6 Compare July 25, 2023 08:04

absoludity force-pushed the react-router-upgrade-2 branch from 8534e7f to 38a6bb5 Compare July 27, 2023 04:20

absoludity force-pushed the react-router-upgrade-2 branch from 91287df to 5b3b9e2 Compare August 2, 2023 05:58

absoludity force-pushed the react-router-upgrade-2 branch from 5b3b9e2 to 1bd24a0 Compare August 2, 2023 06:24

absoludity added 11 commits August 3, 2023 13:07

WIP: working through failig tests... only 165 to go :/

c81decf

Update OperatorNew and PackageView tests. 135 failures to go.

875ec2e

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

LoginForm, PackageView and RequireAuthentication - 128 to go.

4a9db4d

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

Updated tests for OperatorList and OperatorInstanceUpdateForm.

7934eaa

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

Switch remaining tests with updated mountWrapper to help.

a9aea04

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

Lint

ea466b6

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

Update config loader in prep for removing connected router.

5db5f39

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

Remove connected-react-router.

40f5ea6

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

Lint after resolving conflicts.

4dcd699

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

Remove history and add BrowserRouter.

9bfc580

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

Don't dispatch location change on first use.

a67208f

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

absoludity force-pushed the react-router-upgrade-2 branch from 6aae8f6 to a67208f Compare August 3, 2023 06:51

absoludity added 2 commits August 3, 2023 17:03

Fix test.

32deb71

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

Lint

ea6ebde

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

absoludity merged commit 7b4a984 into main Aug 3, 2023
39 checks passed

absoludity deleted the react-router-upgrade-2 branch August 3, 2023 21:03

antgamdia reviewed Aug 4, 2023

View reviewed changes

absoludity added a commit that referenced this pull request Aug 7, 2023

Small changes from review on #6504

948cab7

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

absoludity mentioned this pull request Aug 7, 2023

Small changes from review on #6504 #6571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Actual react-router-dom upgrade #6504

Actual react-router-dom upgrade #6504

absoludity commented Jul 21, 2023 •

edited

netlify bot commented Jul 21, 2023 •

edited

absoludity commented Jul 25, 2023

absoludity commented Jul 28, 2023

absoludity commented Aug 2, 2023

absoludity commented Aug 3, 2023

absoludity commented Aug 3, 2023

antgamdia left a comment •

edited

antgamdia Aug 4, 2023

absoludity Aug 7, 2023

antgamdia Aug 7, 2023

antgamdia Aug 4, 2023

absoludity Aug 6, 2023

antgamdia Aug 4, 2023

absoludity Aug 6, 2023

		@@ -0,0 +1,45 @@
		// Copyright 2018-2023 the Kubeapps contributors.

Actual react-router-dom upgrade #6504

Actual react-router-dom upgrade #6504

Conversation

absoludity commented Jul 21, 2023 • edited

Description of the change

Benefits

Possible drawbacks

Applicable issues

netlify bot commented Jul 21, 2023 • edited

✅ Deploy Preview for kubeapps-dev canceled.

absoludity commented Jul 25, 2023

absoludity commented Jul 28, 2023

absoludity commented Aug 2, 2023

absoludity commented Aug 3, 2023

absoludity commented Aug 3, 2023

antgamdia left a comment • edited

Choose a reason for hiding this comment

antgamdia Aug 4, 2023

Choose a reason for hiding this comment

absoludity Aug 7, 2023

Choose a reason for hiding this comment

antgamdia Aug 7, 2023

Choose a reason for hiding this comment

antgamdia Aug 4, 2023

Choose a reason for hiding this comment

absoludity Aug 6, 2023

Choose a reason for hiding this comment

antgamdia Aug 4, 2023

Choose a reason for hiding this comment

absoludity Aug 6, 2023

Choose a reason for hiding this comment

absoludity commented Jul 21, 2023 •

edited

netlify bot commented Jul 21, 2023 •

edited

antgamdia left a comment •

edited