refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migration #11049

abailly-akamai · 2024-10-05T20:02:09Z

Description 📝

This PR offers a safe pattern to be able to slowly roll out the new Tanstack routing migration.
The worry was to have to replace all of the routes and routing internals at once, resulting in a very large, hard to manage feature branch. Fortunately, this is not the case! The way we can incrementally implement the new routing is to accept to have the two routers cohabitating, and implement a cascading routing approach:

react-router-dom:
- serve non-migrated routes
- route not found: defer to migrationRouter (Tanstack)
tanstack-react-router
- server migrated routes
- route not found: render <NotFound />

The deferring logic lives in <MainContent /> where a wildcard catch-all will defer all unhandled routes to the tanstack <RouterProvider />. The POC is showing that there's no conflict resulting in this approach.

The <Link /> components will be replaced last to avoid any potential conflict and keep things simple (already confirmed that the current behavior is not causing any problem navigating to the new routes 🎆

Lastly, the PR introduces development tools to make out task easier and safer (linting, testing, inspecting)

Changes 🔄

Implement router deferring logic
Migrate /betas paths to the new routing
- remove react-router-dom handling of Betas paths
- replace utils
- switch from using state navigation to params navigation for betas/<detail>
  - improve betas/{id} query to be able to be disabled
- update tests
Add a new e2e smoke test for beta
Update mockServiceWorker declaration
Enable routes migration test
Improve path utils
Add missing lazy imports in a few files to remove build error messages

How to test 🧪

Verification steps

Pull code locally and update dependencies
Confirm the /betas feature has no regression
Confirm application routing behaves well everywhere else
Confirm tests are passing
Confirm being able to use the routing debugging tools in our DevTools

As an Author I have considered 🤔

Check all that apply

packages/manager/.eslintrc.cjs

packages/manager/public/mockServiceWorker.js

abailly-akamai · 2024-10-07T19:19:44Z

packages/manager/src/MainContent.tsx

+                           */}
+                          <Route path="*">
+                            <RouterProvider router={migrationRouter} />
+                          </Route>


This is the primary deferring change described in the PR summary.

abailly-akamai · 2024-10-07T19:21:48Z

packages/manager/src/features/Betas/BetaSignup.tsx

-  const { data: beta, isError, isLoading } = useBetaQuery(betaId);
+  const navigate = useNavigate();
+  const { betaId } = useParams({ from: '/betas/signup/$betaId' });
+  const { data: beta, isError, isLoading } = useBetaQuery(betaId, !!betaId);


Using params rather than state seems to me like an always better pattern - i don't see any drawback of doing so, hopefully i did not miss anything

I had the same thought back when this was implemented #9544 (comment)

Should be fine

abailly-akamai · 2024-10-07T19:23:02Z

packages/manager/src/features/Betas/index.tsx

With this migration we will see most feature/index.tsx deleted since the routing is now centralized

abailly-akamai · 2024-10-07T19:27:15Z

packages/manager/src/queries/betas.ts

+  useQuery({
+    ...betaQueries.beta(id),
+    enabled,
+  });


Just adding the enabled parameter since we now have a URL param to check if we want to run the query or not

abailly-akamai · 2024-10-07T19:27:55Z

packages/manager/src/routes/betas.tsx

  return (
    <React.Suspense fallback={<SuspenseLoader />}>
-      <Outlet />
+      {selfServeBetas ? <Outlet /> : <NotFound />}


moving the <MainContent /> flag logic here, which is where it belongs now

abailly-akamai · 2024-10-07T19:28:49Z

packages/manager/src/routes/types.ts

@@ -8,5 +8,4 @@ export type RouterContext = {
  isACLPEnabled?: boolean;
  isDatabasesEnabled?: boolean;
  isPlacementGroupsEnabled?: boolean;
-  selfServeBetas?: boolean;


Not part of the context anymore since we're check this at the betas root route now

abailly-akamai · 2024-10-07T19:29:17Z

packages/manager/src/routes/utils/allPaths.ts

    })
-  );
+    .filter((path) => path !== '/')
+    .filter(Boolean);


just simplifying this util by using tanstack internals

Nice file cleanup 🧹

packages/manager/src/types/index.ts

github-actions · 2024-10-07T20:02:32Z

Coverage Report: ✅
Base Coverage: 86.96%
Current Coverage: 87.06%

packages/manager/src/features/Betas/BetaDetails.tsx

packages/manager/src/testSetup.ts

packages/manager/src/routes/index.tsx

abailly-akamai · 2024-10-10T21:39:22Z

packages/manager/cypress/e2e/core/account/smoke-enroll-beta.spec.ts

+    cy.url().should('include', '/betas');
+    cy.url().should('not.include', 'signup');
+  });
+});


@jdamore-linode! just wanted to mention you in here to take a quick look at the smoke test here. The feature's e2e coverage wasn't too exhaustive so that should add a lil' bit of confidence

abailly-akamai · 2024-10-10T21:40:09Z

packages/manager/src/features/Betas/BetaDetailsList.test.tsx

@@ -53,6 +63,7 @@ describe('BetaDetails', () => {
    const errorBetasList = renderWithTheme(
      <BetaDetailsList
        betas={[]}
+        dataQA="betas"


prop drilling a bit for testing purposes

abailly-akamai · 2024-10-10T21:40:55Z

packages/manager/src/features/Betas/BetaDetailsList.tsx

+            <Typography>No betas found</Typography>
+          )}
+        </Stack>
+      )}


sorry, this was just an ugly loading experience. just a bit nicer with this update

packages/manager/.eslintrc.cjs

packages/manager/cypress/support/intercepts/betas.ts

packages/manager/src/features/Betas/BetasLanding.tsx

packages/manager/src/routes/root.ts

packages/manager/src/routes/routes.test.tsx

jaalah-akamai · 2024-10-11T14:00:21Z

packages/manager/src/routes/utils/allPaths.ts

    })
-  );
+    .filter((path) => path !== '/')
+    .filter(Boolean);


Nice file cleanup 🧹

packages/manager/src/utilities/testHelpers.tsx

bnussman-akamai

Looks good! 🎉

I was trying to get some "transitionary" type-safety by doing this in our Link component:

But that lead me to find out that that the router's global module override doesn't seem to be working as expected. (See comment in packages/manager/src/routes/index.tsx)

packages/manager/src/routes/routes.test.tsx

packages/manager/src/utilities/testHelpers.tsx

bnussman-akamai · 2024-10-15T14:53:47Z

packages/manager/src/routes/index.tsx

@@ -70,6 +70,26 @@ export const router = createRouter({
 declare module '@tanstack/react-router' {
  interface Register {
    // This infers the type of our router and registers it across the entire project
-    router: typeof router;
+    // router: typeof router;
+    migrationRouter: MigrationRouter;


I believe this is essentially making the router untyped / any.

I tried updating it restore the type-safety:

Suggested change

migrationRouter: MigrationRouter;

router: MigrationRouter;

but this introduces many TypeScript errors because of all of the work we've done for routes that are not yet in the migrationRouter.

Any thoughts here? Will we just have to proceed with an untyped router until the end of this migration?

Maybe we could do something hacky like this, but it might cause us more trouble than we want

Suggested change

migrationRouter: MigrationRouter;

router: typeof router | MigrationRouter;

@bnussman of course! 🤦 thx for catching.

So look at 75b20f0

I reintroduced the main router type for the applciation, which also forced me to cast AnyRouter in a few places. Still much better! and allowed to get <Link /> to show path suggestion which is a good idea.

We still have good typing but will not narrow to the migrationRouter routes which I was hoping to be able to do. We're half way there and get at least stronger rail guards and working suggestions 👍

Nice. Sounds good to me! 🚀

This reverts commit 65653a7.

cypress · 2024-10-16T16:34:58Z

Cloud Manager E2E Run #6684

Run Properties: Failed #6684 • ed30c6e1bb: refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migratio...

Project	`Cloud Manager E2E`
Run status	`Failed #6684`
Run duration	`34m 32s`
Commit	`ed30c6e1bb: refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migratio...`
Committer	`Alban Bailly`
View all properties for this run ↗︎

Test results
Failures	`4`
Flaky	`8`
Pending	`2`
Skipped	`0`
Passing	`433`

Tests for review

oneClickApps/one-click-apps.spec.ts • 1 failed test

View Output Video

Test		Artifacts
OneClick Apps (OCA) > Lists all the OneClick Apps		`Screenshots` `Video`

placementGroups/delete-placement-groups.spec.ts • 1 failed test

View Output Video

Test		Artifacts
Placement Group deletion > can delete with Linodes assigned when unexpected error show up and retry		`Screenshots` `Video`

kubernetes/lke-update.spec.ts • 2 failed tests

View Output Video

Test		Artifacts
LKE cluster updates > can upgrade to high availability		`Screenshots` `Video`
LKE cluster updates > can upgrade kubernetes version from the details page		`Screenshots` `Video`

linodes/clone-linode.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
clone linode > can clone a Linode from Linode details page		`Screenshots` `Video`

objectStorageGen2/bucket-create-gen2.spec.ts • 3 flaky tests

View Output Video

Test		Artifacts
Object Storage Gen2 create bucket tests > can create a bucket with E1 endpoint type		`Screenshots` `Video`
Object Storage Gen2 create bucket tests > can create a bucket with E2 endpoint type		`Screenshots` `Video`
Object Storage Gen2 create bucket tests > can create a bucket with E3 endpoint type		`Screenshots` `Video`

linodes/rebuild-linode.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
rebuild linode > cannot rebuild a provisioning linode		`Screenshots` `Video`

nodebalancers/smoke-create-nodebal.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
create NodeBalancer > displays API errors for NodeBalancer Create form fields		`Screenshots` `Video`

account/account-logout.spec.ts • 1 flaky test

View Output Video

Test		Artifacts
Logout Test > can logout the account and redirect to login endpoint		`Screenshots` `Video`

The first 5 flaky specs are shown, see all 6 specs in Cypress Cloud.

abailly-akamai self-assigned this Oct 5, 2024

abailly-akamai force-pushed the M3-8712 branch from b95457c to abdd0cb Compare October 7, 2024 14:32

abailly-akamai commented Oct 7, 2024

View reviewed changes

abailly-akamai marked this pull request as ready for review October 7, 2024 19:36

abailly-akamai requested a review from a team as a code owner October 7, 2024 19:36

abailly-akamai requested review from hana-akamai and cpathipa and removed request for a team October 7, 2024 19:36

abailly-akamai added the Routing Refactor label Oct 7, 2024

abailly-akamai requested review from bnussman-akamai and jaalah-akamai October 7, 2024 19:45

bnussman-akamai reviewed Oct 7, 2024

View reviewed changes

packages/manager/src/features/Betas/BetaDetails.tsx Outdated Show resolved Hide resolved

packages/manager/src/testSetup.ts Outdated Show resolved Hide resolved

abailly-akamai force-pushed the M3-8712 branch from cc786dd to 2347dcf Compare October 8, 2024 02:54

bnussman-akamai requested a review from a team as a code owner October 8, 2024 16:46

bnussman-akamai requested review from cliu-akamai and removed request for a team October 8, 2024 16:46

bnussman-akamai reviewed Oct 8, 2024

View reviewed changes

packages/manager/src/routes/index.tsx Outdated Show resolved Hide resolved

packages/manager/src/routes/index.tsx Outdated Show resolved Hide resolved

abailly-akamai force-pushed the M3-8712 branch 3 times, most recently from 433a136 to 01fb10a Compare October 10, 2024 13:31

abailly-akamai requested a review from bnussman-akamai October 10, 2024 21:36

abailly-akamai commented Oct 10, 2024

View reviewed changes

jaalah-akamai approved these changes Oct 11, 2024

View reviewed changes

abailly-akamai force-pushed the M3-8712 branch 2 times, most recently from 18004bd to b8b8a93 Compare October 11, 2024 20:40

bnussman-akamai reviewed Oct 14, 2024

View reviewed changes

packages/manager/src/utilities/testHelpers.tsx Outdated Show resolved Hide resolved

abailly-akamai requested a review from bnussman-akamai October 15, 2024 13:07

bnussman-akamai approved these changes Oct 15, 2024

View reviewed changes

bnussman-akamai added the Approved Multiple approvals and ready to merge! label Oct 15, 2024

abailly-akamai and others added 24 commits October 16, 2024 09:05

oops mocking

1442ec5

Wrap up testing

6fb9dd0

Useful comments

2884bf2

moar cleanup

4613912

Added changeset: Migrate /betas routes to Tanstack Router

1152e62

feedback @bnussman-akamai

891fce4

testing migration improvements

37d9e2b

missing import in test

47f2126

fix dynamic import conflicts (hopefully)

02d43fc

temp debug

3b306b4

Revert "temp debug"

1372205

This reverts commit 65653a7.

remove cleanUp and autenticate from longview.spec.ts to temp test

75c4bdc

revert MSW config changes

3bdbaa7

feedback @bnussman-akamai

3420cce

remove det tools

c560831

oof, the testing

ca46aa3

cleanup

16f2421

Type cleanup and more tests

017bcd4

oops revert test experiment

3c7141d

wrapping up: e2e and cleanup

d1ea918

feedback @jaalah-akamai

f5605b9

feedback @bnussman-akamai test routing

1f4a441

oop unskip test

fc5bb98

feedback @bnussman-akamai

e2f2ca8

abailly-akamai force-pushed the M3-8712 branch from 75b20f0 to e2f2ca8 Compare October 16, 2024 13:05

abailly-akamai merged commit ed30c6e into linode:develop Oct 16, 2024
21 of 23 checks passed

abailly-akamai mentioned this pull request Oct 17, 2024

fix: [M3-8758] - Use default v8 reporter in coverage:summary job #11121

Closed

12 tasks

This was referenced Oct 23, 2024

Release v1.131.0 - release → staging #11153

Merged

Release v1.131.0 - staging → master #11175

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migration #11049

refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migration #11049

abailly-akamai commented Oct 5, 2024 •

edited

Loading

abailly-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

bnussman-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

abailly-akamai Oct 7, 2024

jaalah-akamai Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

abailly-akamai Oct 10, 2024

abailly-akamai Oct 10, 2024

abailly-akamai Oct 10, 2024

jaalah-akamai Oct 11, 2024 •

edited

Loading

bnussman-akamai left a comment

bnussman-akamai Oct 15, 2024

abailly-akamai Oct 15, 2024

bnussman-akamai Oct 15, 2024

cypress bot commented Oct 16, 2024

	migrationRouter: MigrationRouter;
	router: typeof router \| MigrationRouter;

refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migration #11049

refactor: [M3-8712] - Tanstack Reat Router rollout setup + betas routes migration #11049

Conversation

abailly-akamai commented Oct 5, 2024 • edited Loading

Description 📝

Changes 🔄

How to test 🧪

Verification steps

As an Author I have considered 🤔

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

jaalah-akamai Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaalah-akamai Oct 11, 2024 • edited Loading

Choose a reason for hiding this comment

bnussman-akamai 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

cypress bot commented Oct 16, 2024

Tests for review

abailly-akamai commented Oct 5, 2024 •

edited

Loading

jaalah-akamai Oct 11, 2024 •

edited

Loading

github-actions bot commented Oct 7, 2024 •

edited

Loading

jaalah-akamai Oct 11, 2024 •

edited

Loading