types(core): allow Promise<void> for provider navigate prop #4098
Conversation
* chore(docs): remove Scrumbuiss * chore(docs): remove Scrumbuiss logo
|
@CanRau is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
|
WalkthroughThe pull request modifies the Changes
Possibly related PRs
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
Documentation and Community
|
![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 (1)
packages/core/system/src/provider.tsx (1)
34-34: LGTM! Consider enhancing the documentationThe type change to support
Promise<void>is well-aligned with modern router implementations and maintains backward compatibility.Consider updating the documentation to reflect the async capability:
/** * Provides a client side router to all nested components such as * Link, Menu, Tabs, Table, etc. + * @param path - The path to navigate to + * @returns void or Promise<void> for async navigation */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/core/system/src/provider.tsx(1 hunks)
🔇 Additional comments (2)
packages/core/system/src/provider.tsx (2)
34-34: Implementation looks solid and non-breaking
The type enhancement to support Promise<void> is well-implemented:
- Maintains backward compatibility with synchronous navigation
- Enables support for async routers like TanStack Router
- Doesn't affect runtime behavior
Line range hint 63-69: Verify RouterProvider compatibility with Promise
The RouterProvider from @react-aria/utils needs to properly handle async navigation.
✅ Verification successful
RouterProvider from @react-aria/utils is compatible with Promise navigation
The implementation is safe because:
- The
navigateprop type in the provider explicitly supportsPromise<void>return type - RouterProvider from @react-aria/utils is used as a pass-through component that simply provides the navigation context
- The documentation shows examples of integration with various routers including TanStack Router which supports async navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check RouterProvider implementation for Promise handling
ast-grep --pattern 'RouterProvider = function($_) {
$$$
navigate($$$)
$$$
}'
# Check if there are any existing usages of async navigation
rg -l "navigate.*Promise" --type ts --type tsx
Length of output: 161
Script:
#!/bin/bash
# Search for RouterProvider implementation with broader pattern
ast-grep --pattern 'RouterProvider = $_'
# Search for RouterProvider class or interface definition
ast-grep --pattern 'interface RouterProvider {
$$$
}'
# Search for navigate type definitions and usages with Promise
rg "navigate.*Promise" -A 3 -B 3
# Search specifically in TypeScript files
rg "navigate.*Promise" --type ts -A 3 -B 3
Length of output: 1332
Script:
#!/bin/bash
# Search for RouterProvider component implementation
rg "RouterProvider" -A 10 -B 10 --type tsx
# Look for any router-related type definitions
rg "type.*Router|interface.*Router" -A 5 -B 5 --type ts
# Search for navigate prop usage in components
ast-grep --pattern 'navigate={$_}'
Length of output: 165
Script:
#!/bin/bash
# Search for RouterProvider in all files
rg "RouterProvider" -A 10 -B 10
# Search for navigate prop usage in all files
rg "navigate={" -A 3 -B 3
# Look for router-related imports
rg "import.*Router" -A 2 -B 2
Length of output: 11173
|
@wingkwong please review |
|
Oh wow didn't realize, just made this change here in the GitHub editor, I'll have a look a little later |
|
@CanRau I think the reason is your also please create a changeset. |
|
Alright, sorry for the delay I'll look into it tomorrow (Monday) |
|
Redid the PR cause it was quicker 😇 |
📝 Description
At least TanStack Router's
router.navigatereturnsPromise<void>so if this PR won't break anything would be nice to support both return types ❤️⛳️ Current behavior (updates)
The
to: any, options: anyis another topic I think🚀 New behavior
No type errors
💣 Is this a breaking change (Yes/No):
Shouldn't break anything I think 🤔
Summary by CodeRabbit
navigatefunction to support asynchronous operations by allowing it to return a promise.This change improves flexibility in navigation handling within the application.