-
Notifications
You must be signed in to change notification settings - Fork 394
feat(expo,react): Make Signal hooks the default #6946
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: vincent-and-the-doctor
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
"react/use-session-list.mdx", | ||
"react/use-session.mdx", | ||
"react/use-sign-in-signal.mdx", | ||
"react/use-sign-in-1.mdx", |
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.
Is this something that we control ? what does 1
mean ?
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.
This is for Typedoc. Since there are now two useSignIn
hooks (one from the root and one from the legacyit adds the
-1` suffix. I don't know if this is controllable.
<ClerkProvider | ||
routerPush={(to: string) => router.push(to)} | ||
routerReplace={to => router.replace(to)} | ||
clerkJSUrl={process.env.EXPO_PUBLIC_CLERK_JS_URL} |
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.
is this updated on purpose ?
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.
Yes, previously the expo tests used the production version of clerk-js rather than the version from the current PR.
// setPendingVerification(true); | ||
// } catch (err: any) { | ||
// // See https://clerk.com/docs/custom-flows/error-handling | ||
// // for more info on error handling | ||
// console.error(JSON.stringify(err, null, 2)); | ||
// } |
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.
🧹
if (clerkExpoPath?.startsWith('link:')) { | ||
return clerkExpoPath.replace('link:', ''); | ||
} | ||
|
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.
🤔 what does this unlock ?
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.
This is for local testing. Locally the package.json has the link:
protocol. However I'm unable to get the tests to work correctly locally 😓
@@ -1,4 +1,4 @@ | |||
import { useSignIn, useSignUp } from '@clerk/react'; | |||
import { useSignIn, useSignUp } from '@clerk/react/legacy'; |
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.
Is this just for this PR, or are we leasing the package like that ?
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.
As of right now this is the intent. If we have time I'll take a look and see if we need to update the API of the useOAuth
hook.
'use client'; | ||
|
||
import { useSignUp } from '@clerk/nextjs'; | ||
import { useSignUp } from '@clerk/nextjs/legacy'; |
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.
Are we creating a new use-sign-up.md
that wll use the new hook ?
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.
This is used in our typedoc comments. Our new comments don't (yet) use this example, so I haven't created one yet.
Description
This PR moves our experimental
useSignInSignal
anduseSignUpSignal
hooks into their rightful place as the defaultuseSignIn
anduseSignUp
hooks. It moves the previous hooks to a@clerk/react/legacy
export. It also updates Expo's use of the legacy hooks to the new export.It adds the
/legacy
export to the Next.js, React Router, Tanstack Start, and Expo SDKs.It also deletes the
@clerk/remix
package and all supporting files that referenced it.Checklist
pnpm test
runs as expected.pnpm build
runs as expected.Type of change