-
Notifications
You must be signed in to change notification settings - Fork 366
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.
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
refactor: [M3-8623] - Introduce TanStack Router #10997
Conversation
3776dd9
to
c16e5eb
Compare
0d4ba5c
to
ca73fee
Compare
maxWidth: '100%', | ||
position: 'relative', | ||
}, | ||
})); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All existing styles from <MainContent />
extracted to their own file - this also isn't affecting the current experience
</SessionExpirationProvider> | ||
</div> | ||
); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decoupled the root from the so the saparation of concern between routing and composition is more clear. This is not affecting the current experience (unless switching things in <App />
manually)
}); | ||
|
||
return <RouterProvider router={router} />; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Our new entrypoint to load all routes in Cloud Manager (not active in this PR!)
'/account-activation' | ||
)({ | ||
component: AccountActivationLanding, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All createLazyRoute
exports you will see in this PR aim to facilitate lazy loading smaller bundles: https://tanstack.com/router/latest/docs/framework/react/api/router/createLazyRouteFunction#createlazyroute-function
Those get imported in the /routes/* files via .lazy()
Coverage Report: ✅ |
sx={{ paddingLeft: theme.spacing(1) }} | ||
variant="h2" | ||
> | ||
Invoice #{invoice.id} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the removal of the #
is causing an e2e test failure
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah yes thank you, this change is unrelated but we did not have any h1 on this page so i thought to add one (was helpful during my testing to have an H1 on each page for assertions that the page is rendering something within the route. I'll fix it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall this is amazing work 🔥 - I still want to look deeper into this, but here's some minor stuff I found.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall structure and patterns look good to me!
21f7a5e
to
8f5f1a1
Compare
Cloud Manager E2E Run #6623
Run Properties:
|
Project |
Cloud Manager E2E
|
Branch Review |
develop
|
Run status |
Passed #6623
|
Run duration | 26m 06s |
Commit |
ccb4e0bd9a: refactor: [M3-8623] - Introduce TanStack Router (#10997)
|
Committer | Alban Bailly |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
3
|
Pending |
2
|
Skipped |
0
|
Passing |
414
|
View all changes introduced in this branch ↗︎ |
Description 📝
This (large) PR introduces
TanStack Router
to Cloud Manager.https://tanstack.com/router/latest
It prepares a full migration from our old, outdated v5
react-router-dom
to the new Routing API.The benefits of this migration are multiple:
Important
This initial PR does not (read should not) affect the current Cloud Manager routing experience. It only aims to achieve the following:
I do not expect to have 100% route parity with this PR, I however did my very best get as close as possible. Subsequent PRs will rollout the actual new routing by updating legacy utils with the TanStack ones (params, location, history, etc)
Changes 🔄
How to test 🧪
Prerequisites
yarn
to get new dependencyVerification steps
<App />
to very basic functionalityAs an Author I have considered 🤔
Check all that apply