fix static generation issue #262
Conversation
yyaskriloff
requested review from
DaveOrDead and
peterphanouvong
as code owners
WalkthroughThe pull request modifies the Changes
Sequence DiagramsequenceDiagram
participant Func as Session Function
participant Conn as Connection
Func->>Conn: await connection()
Conn-->>Func: Connection resolved
Func->>Func: Continue with session operations
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
I changed from using headers to the connection function. i saw that noStore was legacy but missed to use connection instead |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/session/index.ts (2)
Line range hint
28-33: Document RSC limitations in API documentationThe code comment about React Server Components (RSC) limitations with
refreshTokensprovides valuable information. Consider documenting this limitation in the package's API documentation to help users avoid potential issues.Let's check if this limitation is already documented:
#!/bin/bash # Search for existing documentation about RSC limitations rg -g "*.md" -g "*.mdx" "React Server Components.*refreshTokens|RSC.*refreshTokens"
23-24: Consider alternative approaches to static generationWhile using
connection()fromnext/servermight work, consider documenting why this approach was chosen over other alternatives mentioned in the PR description. This would help maintainers understand the trade-offs made.Additionally, since this is a breaking change, consider:
- Adding migration guides in the documentation
- Updating the version number according to semver
- Adding deprecation warnings for the current synchronous usage
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/session/index.ts(2 hunks)
🔇 Additional comments (1)
src/session/index.ts (1)
21-21: Breaking Change: Function signature updated to asyncThis change will require all instances of
getKindeServerSessionto be called withawait. While this aligns with the PR objective to fix static generation, we should verify if this is the recommended approach.Let's check for any existing usage patterns in the codebase:
Also applies to: 23-23
✅ Verification successful
Breaking change is intentional and aligned with existing patterns
The change to make
getKindeServerSessionasync is:
- Already anticipated in the codebase comments
- Partially implemented in some routes (e.g., api/protected/route.ts)
- Required for proper static generation as per PR objectives
Locations requiring updates:
playground/src/app/layout.tsx: Update to use await withgetKindeServerSession()🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existing getKindeServerSession usage patterns rg "getKindeServerSession" -A 2Length of output: 1700
|
Nitpick but I think adding a comment in the code as to why we're doing this would be valuable here. Making |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/session/index.ts (1)
24-25: Enhance the comment to explain the rationale.While the comment explains what the code does, it would be helpful to add:
- Why
connection()was chosen over other approaches- The implications of this change on the build process
- Any potential future plans to remove this once
getKindeServerSessionis fully rewritten as asyncConsider expanding the comment like this:
- // This will prevent NextJS generating a static page where this function is called if no other Dynamic APIs are used. + // Using connection() to prevent NextJS from generating a static page where this function is called. + // This approach was chosen over alternatives because: + // 1. It's a documented Next.js API + // 2. It ensures consistent behavior across different Next.js versions + // Note: This might be removable once getKindeServerSession is fully rewritten as async
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/session/index.ts(2 hunks)
🧰 Additional context used
📓 Learnings (1)
src/session/index.ts (1)
Learnt from: yyaskriloff
PR: kinde-oss/kinde-auth-nextjs#262
File: src/session/index.ts:24-24
Timestamp: 2025-01-22T13:04:16.055Z
Learning: The `connection()` function from 'next/server' is a documented Next.js API (https://nextjs.org/docs/app/api-reference/functions/connection) that can be used to detect when a connection with a client is lost/closed in Next.js applications.
🔇 Additional comments (2)
src/session/index.ts (2)
21-21: LGTM!The import of
connectionfrom 'next/server' is correctly added and is necessary for preventing static page generation.
34-34: LGTM!The addition of
awaitfor thesessionManagercall is consistent with the async function signature change.
|
|
||
| export default function (req?: NextApiRequest, res?: NextApiResponse) { | ||
| export default async function (req?: NextApiRequest, res?: NextApiResponse) { |
There was a problem hiding this comment.
💡 Codebase verification
Document the breaking change in function signature
The conversion of getKindeServerSession to async is a breaking change that requires all calls to be updated with await. While the playground examples demonstrate the correct usage, this breaking change should be explicitly documented in the CHANGELOG.md to help users understand the required migration steps.
Locations needing attention:
- CHANGELOG.md: Add entry under v2.4.2 to document the breaking change in
getKindeServerSession - Consider adding a migration guide to help users update their code
🔗 Analysis chain
Document the breaking change in function signature.
Converting this function to async is a breaking change that requires all getKindeServerSession calls to be updated with await. This should be documented in the changelog and migration guide.
Let's verify the impact of this breaking change:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing getKindeServerSession usage that might need updates
rg "getKindeServerSession" --type ts
Length of output: 711
Script:
#!/bin/bash
# Check the content of files using getKindeServerSession
echo "=== playground/src/app/layout.tsx ==="
cat playground/src/app/layout.tsx
echo -e "\n=== playground/src/app/api/protected/route.ts ==="
cat playground/src/app/api/protected/route.ts
Length of output: 3300
Script:
#!/bin/bash
# Check for changelog or migration guide
echo "=== Checking for CHANGELOG.md ==="
fd "CHANGELOG.md"
echo -e "\n=== Checking for migration guide ==="
fd -i "migration" -e md
Length of output: 220
Script:
#!/bin/bash
# Check CHANGELOG.md content
cat CHANGELOG.md
Length of output: 65555
addressing issue #261
I had a hard time building the playground so I couldn't test building. also if it's not obvious this would be a breaking change because every call of
getKindeServerSessionwould need to be switched to awaitI chose headers but it can be accomplished with multiple different functions for more info https://nextjs.org/docs/app/building-your-application/rendering/server-components#dynamic-apis
🛟 If you need help, consider asking for advice over in the Kinde community.