fix: withUnkey should not fail build (@unkey/nextjs) #2651
Conversation
harshsbhat
requested review from
perkinsjr,
chronark,
mcstepp and
MichaelUnkey
as code owners
🦋 Changeset detectedLatest commit: 5b3f80a The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
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 (
|
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
@harshsbhat is attempting to deploy a commit to the Unkey Team on Vercel. A member of the Team first needs to authorize it. |
|
Thank you for following the naming conventions for pull request titles! 🙏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/nextjs/src/index.ts (2)
Line range hint
58-62: Add documentation for async params handling.While the implementation correctly handles the async context, it would be helpful to add JSDoc comments explaining that
paramsare now asynchronous in Next.js 15.Add documentation like this:
export function withUnkey<TContext extends NextContext = NextContext>( + /** + * @param handler - Request handler function + * @param config - Configuration options + * @remarks Since Next.js 15, context.params is asynchronous and must be awaited + * @example + * ```ts + * export default withUnkey(async (req, context) => { + * const params = await context.params; + * // ... rest of the handler + * }); + * ``` + */ handler: ( req: NextRequestWithUnkeyContext, context: TContext, ) => Response | NextResponse | Promise<Response | NextResponse>, config?: WithUnkeyConfig,
Line range hint
63-106: LGTM: Implementation handles async context correctly.The function implementation:
- Properly maintains type safety with the generic constraint
- Preserves existing error handling
- Correctly handles the async nature of the context
Consider adding a middleware or utility function to handle common params extraction patterns, making it easier for users to work with the async params. This could be a future enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/nextjs/src/index.ts(1 hunks)
🔇 Additional comments (1)
packages/nextjs/src/index.ts (1)
55-55: LGTM: Type modification aligns with Next.js 15 requirements.
The change correctly updates NextContext to handle params as a Promise, addressing the core issue with Next.js 15 compatibility.
Let's verify there are no other instances of synchronous params usage:
✅ Verification successful
Let me try a more focused search to find actual JavaScript/TypeScript code that uses context.params:
Let me try one more search using ast-grep to find any usage of the NextContext type or params access patterns:
Let me correct the ast-grep command with proper language specification:
Let me try one final search focusing on the specific file and its imports:
Type modification for NextContext is correctly implemented and isolated
The change to make params a Promise in the NextContext type is correctly implemented and doesn't impact other parts of the codebase because:
- The type is only used as a generic constraint in the
withUnkeyfunction - The
paramsfield is never directly accessed in the implementation - The type is properly exported for consumers to use with Next.js 15
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any direct params access that might need Promise handling
rg -l "context.params" | xargs ast-grep --pattern "context.params" -A 3
Length of output: 20177
Script:
#!/bin/bash
# Search for TypeScript/JavaScript files containing context.params
rg -t ts -t js "context.params" -A 3
Length of output: 36
Script:
#!/bin/bash
# Search for NextContext type usage and any params access patterns
ast-grep --pattern 'NextContext' -l
ast-grep --pattern 'const { params } = context' -l
ast-grep --pattern 'await context.params' -l
Length of output: 453
Script:
#!/bin/bash
# Search for NextContext type usage and any params access patterns in TypeScript files
ast-grep --lang ts --pattern 'NextContext' -l
ast-grep --lang ts --pattern 'const { params } = context' -l
ast-grep --lang ts --pattern 'await context.params' -l
Length of output: 483
Script:
#!/bin/bash
# Check the content of the file and any related imports
cat packages/nextjs/src/index.ts
Length of output: 3345
harshsbhat
changed the title
|
I tried this by locally setting up the |
What does this PR do?
Fixes #2554
After Nextjs 15 the
@unkey/nextjsneeds to have a promise for the paramsType of change
How should this be tested?
withUnkeyin nextjs 15 applicationnpm run buildor build command with your preferred package managerChecklist
Required
pnpm buildpnpm fmtconsole.logsgit pull origin mainAppreciated
Summary by CodeRabbit
New Features
NextContextto support asynchronous parameter resolution.@unkey/nextjspackage to align with Next.js version 15 for improved type compatibility.Bug Fixes
withUnkeyfunction for better response management.