fix(system): allow Promise<void> for provider navigate prop #4112
Conversation
|
@CanRau is attempting to deploy a commit to the NextUI Inc Team on Vercel. A member of the Team first needs to authorize it. |
🦋 Changeset detectedLatest commit: d6f9998 The changes in this PR will be included in the next version bump. This PR includes changesets to release 20 packages
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 |
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
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! Type change correctly implements Promise supportThe updated type signature
(path: string) => void | Promise<void>is a well-implemented solution that:
- Maintains backward compatibility with existing synchronous navigate functions
- Adds support for async navigation handlers like TanStack Router
- Implements a non-breaking change through type expansion
This change follows TypeScript best practices by using a union type to support both synchronous and asynchronous patterns, making the component more flexible for different routing solutions.
|
like so? |
wingkwong
changed the title
|
I see, like this? 😇 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
.changeset/green-donuts-reply.md (1)
4-5: Consider enhancing the changeset description.While the current description is clear, it would be helpful to mention that this change specifically improves compatibility with TanStack Router's
router.navigatemethod. This context helps other developers understand the motivation behind the change.Consider updating the description to:
-allow Promise<void> for provider navigate prop +allow Promise<void> for provider navigate prop to support TanStack Router's router.navigate
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
.changeset/green-donuts-reply.md(1 hunks)
🔇 Additional comments (1)
.changeset/green-donuts-reply.md (1)
1-3: LGTM! Patch version is appropriate.
The patch version bump is correct as this change only enhances type definitions without breaking existing functionality.
|
That's why it's returning You mean like so? |
|
Actually this interface RouterProviderProps {
navigate: (path: Href, routerOptions: RouterOptions | undefined) => void;
useHref?: (href: Href) => string;
children: ReactNode;
}And the parameters got changed so I agree we should update that part only. |
|
Closing this PR.
|
|
I see, thank you and sorry for the delay was more packed than anticipated 😵💫 |
Redid #4098 to get rid of all those unrelated changes
📝 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
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.