-
Notifications
You must be signed in to change notification settings - Fork 425
feat: add withPageAuthRequired
for protecting pages client side
#2193
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
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2193 +/- ##
==========================================
+ Coverage 82.91% 83.96% +1.05%
==========================================
Files 21 22 +1
Lines 2095 2133 +38
Branches 372 386 +14
==========================================
+ Hits 1737 1791 +54
+ Misses 351 336 -15
+ Partials 7 6 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Can we add docs in README.md and EXAMPLES.md too?
@tusharpandey13 The Is there anything in particular that you see missing we should include? |
my bad, missed the diff for EXAMPLES.md, this look good 👍 |
const currentLocation = window.location; | ||
returnToPath = pathname + currentLocation.search + currentLocation.hash; |
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.
What's the reason not to use this?
const currentLocation = window.location.toString();
returnToPath = currentLocation.replace(new URL(currentLocation).origin, '') || '/';
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.
window.location
contains the complete URL path which also includes the base path — this means we would end up with /dashboard/dashboard/profile
if the base path was configured as /dashboard
and had a returnTo
of /dashboard/profile
.
We would have to handle this manually, if a base path is configured, which was the first approach I went with: #2193 (comment)
Ultimately, I decided it's cleaner to let Next.js handle this via usePathname
since it does not include the base path which avoids having to manually handle the removal of the base path.
📋 Changes
Adds the
withPageAuthRequired
HoC to protect pages, client side.withPageAuthRequired
for SSR and APIs will follow in subsequent PRs.📎 References
🎯 Testing
withPageAuthRequired
(see sample below)