chore: `react-router-dom` migration #980

w1kman · 2024-11-01T12:37:47Z

What this PR does / why we need it:
Update code base to use react-router-dom@v6 APIs with react-router-dom-v5-compat.
v5 is obsolete and Grafana now bundles with v6 (opt-in, actually react-router-dom-v5-compat).

With these changes, SM removes a lot of code debt tied to react-router-dom.

This PR also updates routing with new paths and some and a NotFound component.

Route changes
From /checks/edit/<id>/<checkTypeGroup> to /checks/<id>/edit
From /checks/<id>/dashboard to /checks/<id>
From /probes/edit/<id> to /probes/<id>/edit (edit) | /probes/<id> (view)

Not found page

/checks/2230/not-a-valid-route

Note: An invalid check ID in the route will keep the old (redirect) behavior (for now).

/checks/new/not-a-valid-check-type-or-group

/probes/<invalid-id>

View mode for private probes
There is now a View mode for the private probe edit page

Those who can edit can click the edit button to switch to edit mode. Those without sufficient rights to edit, will not see the button.

Breadcrumbs
Attempts have been made to improve breadcrumbs (mainly checks and probes routes).

Which issue(s) this PR fixes:
Resolves: #854
Resolves: #975

Special notes for your reviewer:
Getting tests to work again was a battle. Pay extra attention to strange test setup etc (very likely there are some exploratory code in the code base).

Try to make the routes break! 🙇🏻

…mpat` - update routes - remove unused code - add `NotFound` page (wip)

- fix type issues - remove redundant variable - fix tests (most of them)

- fix bug in `useSearchQueryParametersState`

- downgrade packages to match grafana `grafanaDependency` version

- use `generateRoutePath` - remove done TODOs - update tests for `ProbeCard` - update tests for `NewProbe` - update tests for `EditProbe` - remove debug content from `TestRouteInfo`

w1kman

self-review ✅

src/components/CheckForm/CheckForm.tsx

src/components/CheckList/CheckList.test.tsx

src/components/ProbeCard/ProbeCard.test.tsx

src/page/EditCheck/__tests__/ApiEndPointChecks/HTTPCheck/Submit.test.tsx

src/page/EditCheck/__tests__/ApiEndPointChecks/TCPCheck/Submit.test.tsx

src/page/EditProbe/EditProbe.test.tsx

src/page/NewProbe/NewProbe.test.tsx

src/routes/test/TestRouteInfo.tsx

- update not found usage for `EditProbe`

ckbedwell

This is so good and is paying off a ton of debt. I didn't even find the 79 files that daunting because of how much good work it was doing 🙏🙏🙏

Main asks are a few of the mundane dev tasks: tests and documentation.

The only thing I didn't get a chance to test before I wrap up for the week (and go on PTO for another week) is what happens with all the existing routes and if the redirects are handled correctly. I saw some legacy redirect logic but didn't get a chance to test it.

.config/jest-setup.js

ckbedwell · 2024-11-08T16:28:49Z

package.json

    "@grafana/scenes": "5.1.0",
    "@grafana/schema": "11.0.0",
-    "@grafana/ui": "11.0.0",
+    "@grafana/ui": "^11.0.0",


I'd make the argument we should go the other way and get rid of all of the caret flag syntax in package.json and be explicit so this acts more closely as the source of truth rather than having to grep search the yarn.lock file to check what version is actually being used. (@grafana/ui 11.3.0 in this case).

Also, in this case we have to update the grafanaDependency in plugin.json to >=11.3.0 to be on the safe side (and remember to check if we should bump it every time we do yarn install).

I also don't trust developers enough to respect npm semvar... (I mean just look at our plugin releases).

Happy to debate if you think we should keep caret syntax, though.

Unfortunately, 11.3.0 is required for the tests not to blow up.
So, unless it's absolutely crucial that we stay on an older version of @grafana/ui, I'd rather bump the dep version to 11.3.0

Sorry, I should have been clearer. I am all for updating the packages -- little and often is great and if we can continually update the Grafana packages especially so they are as close to the runtime as possible that's great.

My concern is in utilising the ^ caret syntax for the dependencies. I'm not a fan of it because package.json becomes opaque and you have to delve into yarn.lock to know what you are really using. It also assumes that every package you utilise accurately follows semver and I have trust issues after this being a source of bugs in other projects I have worked on previously 😅

I can see why pinning @grafana/* versions makes sense (even though it doesn't mean that the code will run with that version in the wild), but pinning "other" packages for no other reason than "what if..." seems like a false sense of security. Many of our dependencies have their own dependencies that will install minor and patch, meaning that we have no actual control over potential side-effects by installing a minor/patch.
Also, when two (including our package) or more packages rely on the same major version, they may share the same package instead of installing their specific version.

Note: is bundleGrafanaUI: false something we want to use? I think it ties into this.

TL;DR: I could live with (but, wouldn't recommend) pinned versions with ~ (so that patches are installed).

Alright, let's agree to pin the runtime @grafana packages and update the plugin.json grafanaDependency to match.

We haven't encountered any problems with the caret syntax and you are right, it does help manage the dependency tree better.

Note: is bundleGrafanaUI: false something we want to use? I think it ties into this.

No, I don't think we do. It just creates a different set of problems and goes against the grain of what everyone else is doing. Because we have SLOs with CVEs now we should naturally get a pretty good rhythm of keeping our dependencies updated.

src/components/CheckList/CheckList.test.tsx

ckbedwell · 2024-11-08T17:18:28Z

src/components/ProbeEditor/ProbeEditor.tsx

@@ -31,9 +32,11 @@ export const ProbeEditor = ({
  probe,
  submitText,
  supportingContent,
+  readOnly,


Can we add a few tests as a safety and implicit documentation for the readOnly prop?

My worry is reading this in the future it is confusing there is a readOnly prop and also a useCanEditProbe hook so it looks like something is redundant and can be removed.

I think much of the confusion stems from the component names. Should we make a ViewProbe component to add to the router? Maybe we can tackle that when we do #848?

updated the prop name forceViewMode

added test for the new prop

This is an intermediate solution for the lack of a ViewProbe component.

src/page/NewProbe/NewProbe.test.tsx

ckbedwell · 2024-11-08T17:41:48Z

src/test/render.tsx

+                        path={route}
+                        element={
+                          <>
+                            <TestRouteInfo />


Can we document what this is doing, please. It took me a while to work out its purpose and had to look at some of the tests. Would help me out on getting started on this ticket, too: #875 🙏

I added some JSDoc to the component.
Depending on your IDE, it should allow for some additional clarification

Using JetBrains (GoLand)

📜🥳 🎉

VikaCep

This looks really great 🎉🎉🎉
Thank you for putting the time into this! I know fixing the tests wasn’t trivial 🙏
I’ve left a few comments and questions, but overall, everything looks fantastic. I tried to make the routes break, but I wasn't successful 💯

src/components/CheckEditor/FormComponents/ChooseCheckType.tsx

src/components/CheckForm/CheckForm.tsx

VikaCep · 2024-11-11T18:24:27Z

src/components/CheckForm/useCheckType.tsx

+  const { checkTypeGroup } = useParams<CheckFormPageParams>();
+
+  if (check) {
+    const checkType = getCheckType(check.settings);
+    return CHECK_TYPE_OPTIONS.find(({ value }) => value === checkType)?.group;
+  }
+
+  return checkTypeGroup;


Suggested change

const { checkTypeGroup } = useParams<CheckFormPageParams>();

if (check) {

const checkType = getCheckType(check.settings);

return CHECK_TYPE_OPTIONS.find(({ value }) => value === checkType)?.group;

}

return checkTypeGroup;

if (check) {

const checkType = getCheckType(check.settings);

return CHECK_TYPE_OPTIONS.find(({ value }) => value === checkType)?.group;

}

const { checkTypeGroup } = useParams<CheckFormPageParams>();

return checkTypeGroup;

@VikaCep are you we want to do this?
We'd have to add // eslint-disable-next-line react-hooks/rules-of-hooks before the conditional hook.

ESLint: React Hook "useParams" is called conditionally. React Hooks must be called in the exact same order in every component render. Did you accidentally call a React Hook after an early return?(react-hooks/ rules-of-hooks)

VikaCep · 2024-11-11T18:26:51Z

src/page/NewCheck/NewCheck.tsx

 import { CHECK_TYPE_GROUP_OPTIONS } from 'hooks/useCheckTypeGroupOptions';
 import { CheckForm } from 'components/CheckForm/CheckForm';
+import { getRoute } from 'components/Routing.utils';
+
+import { PluginPageNotFound } from '../NotFound/NotFound';

 export const NewCheck = () => {
  const { checkTypeGroup } = useParams<CheckFormPageParams>();


should we use const checkTypeGroup = useFormCheckTypeGroup(); here instead?

IMO, using useFormCheckTypeGroup doesn't add anything. The use of useParams is a fallback mechanism in useFormCheckTypeGroup (for when check is undefined).

src/components/Routing.tsx

VikaCep · 2024-11-11T19:37:53Z

src/page/ChecksPage.test.tsx

+    <QueryClientProvider client={getQueryClient()}>
+      <MetaContextProvider meta={{ ...SM_META, ...meta }}>
+        <FeatureFlagProvider>
+          <SMDatasourceProvider>
+            <PermissionsContextProvider>
+              <MemoryRouter initialEntries={history.entries}>
+                <CompatRouter>
+                  <Routes>
+                    <Route path={PLUGIN_URL_PATH}>
+                      <Route path="*" element={children} />
+                    </Route>
+                  </Routes>
+                </CompatRouter>
+              </MemoryRouter>
+            </PermissionsContextProvider>
+          </SMDatasourceProvider>
+        </FeatureFlagProvider>
+      </MetaContextProvider>
+    </QueryClientProvider>
+  );
+}


😅😅😅

VikaCep · 2024-11-11T19:43:07Z

src/test/fixtures/datasources.ts

+      extensions: {
+        exposedComponents: [],
+      },


Just wondering, but why was this needed?

Updated types require this.

- lock grafana dependencies on 11.3.0 - add 404 page instead of redirect to Home

# Conflicts: # package.json # yarn.lock

w1kman added 8 commits November 1, 2024 13:35

chore: migrate to react-router-dom@v6 (with `react-router-dom-v5-co…

65c53c8

…mpat` - update routes - remove unused code - add `NotFound` page (wip)

chore: update summary table links

af7d9a3

chore: create-plugin update

652ca8d

- fix type issues - remove redundant variable - fix tests (most of them)

chore: fix more tests

afa8f54

- fix bug in `useSearchQueryParametersState`

chore: redirect to home

0441a35

chore: update to node v20

b733f02

- downgrade packages to match grafana `grafanaDependency` version

fix: add missing line break

a008ce4

fix: self-review change requests

8fd524a

- use `generateRoutePath` - remove done TODOs - update tests for `ProbeCard` - update tests for `NewProbe` - update tests for `EditProbe` - remove debug content from `TestRouteInfo`

w1kman commented Nov 5, 2024

View reviewed changes

fix: self-review change requests

63a338b

- update not found usage for `EditProbe`

w1kman marked this pull request as ready for review November 5, 2024 20:57

w1kman requested a review from a team as a code owner November 5, 2024 20:57

w1kman requested review from VikaCep and ckbedwell November 5, 2024 20:58

w1kman self-assigned this Nov 7, 2024

ckbedwell requested changes Nov 8, 2024

View reviewed changes

VikaCep reviewed Nov 11, 2024

View reviewed changes

fix: pr change requests

31673f6

- lock grafana dependencies on 11.3.0 - add 404 page instead of redirect to Home

w1kman requested review from VikaCep and ckbedwell November 14, 2024 15:50

Merge branch 'main' into chore/update-react-router-dom

7cd1436

# Conflicts: # package.json # yarn.lock

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: `react-router-dom` migration #980

chore: `react-router-dom` migration #980

w1kman commented Nov 1, 2024 •

edited

Loading

w1kman left a comment

ckbedwell left a comment

ckbedwell Nov 8, 2024

w1kman Nov 14, 2024

ckbedwell Nov 18, 2024

w1kman Nov 19, 2024

ckbedwell Nov 19, 2024

ckbedwell Nov 8, 2024

w1kman Nov 14, 2024

ckbedwell Nov 8, 2024

w1kman Nov 14, 2024

ckbedwell Nov 18, 2024

VikaCep left a comment

VikaCep Nov 11, 2024

w1kman Nov 14, 2024

VikaCep Nov 11, 2024

w1kman Nov 14, 2024

VikaCep Nov 11, 2024

VikaCep Nov 11, 2024

w1kman Nov 14, 2024 •

edited

Loading

chore: react-router-dom migration #980

Are you sure you want to change the base?

chore: react-router-dom migration #980

Conversation

w1kman commented Nov 1, 2024 • edited Loading

w1kman left a comment

Choose a reason for hiding this comment

ckbedwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VikaCep left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

w1kman Nov 14, 2024 • edited Loading

Choose a reason for hiding this comment

chore: `react-router-dom` migration #980

chore: `react-router-dom` migration #980

w1kman commented Nov 1, 2024 •

edited

Loading

w1kman Nov 14, 2024 •

edited

Loading