-
Notifications
You must be signed in to change notification settings - Fork 426
feat: add withPageAuthRequired
for server
#2207
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2207 +/- ##
==========================================
+ Coverage 84.23% 84.71% +0.48%
==========================================
Files 22 24 +2
Lines 2208 2284 +76
Branches 397 420 +23
==========================================
+ Hits 1860 1935 +75
- Misses 342 343 +1
Partials 6 6 ☔ 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.
These changes look good. I have a small suggestion:
getServerSideProps
can return 404 or a redirect object too, see https://nextjs.org/docs/pages/api-reference/functions/get-server-side-props#getserversideprops-return-values
I don't think we have tests for these.
Thanks for taking a look @tusharpandey13. Since the SDK is the one returning the |
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.
lgtm
📋 Changes
Adds
withPageAuthRequired
for server-rendered components.📎 References
Fixes: #1790
🎯 Testing
See
EXAMPLES.md
for testable examples using the App and Pages router.