-
Notifications
You must be signed in to change notification settings - Fork 461
feat(clerk-js,clerk-react,types): Signal errors #6495
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
Changes from 1 commit
837fdc2
7f1dcc9
5256fc2
4b3f44f
4d9b742
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| --- | ||
| '@clerk/clerk-js': minor | ||
| '@clerk/clerk-react': minor | ||
| '@clerk/types': minor | ||
| --- | ||
|
|
||
| [Experimental] Signal Errors |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ import type { | |||||||||||||||||||||
| AuthenticateWithPopupParams, | ||||||||||||||||||||||
| AuthenticateWithRedirectParams, | ||||||||||||||||||||||
| AuthenticateWithWeb3Params, | ||||||||||||||||||||||
| ClerkAPIResponseError, | ||||||||||||||||||||||
| CreateEmailLinkFlowReturn, | ||||||||||||||||||||||
| EmailCodeConfig, | ||||||||||||||||||||||
| EmailLinkConfig, | ||||||||||||||||||||||
|
|
@@ -557,7 +558,7 @@ class SignInFuture implements SignInFutureResource { | |||||||||||||||||||||
| action: 'prepare_first_factor', | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| } catch (err: unknown) { | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError }); | ||||||||||||||||||||||
| return { error: err }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
560
to
562
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Avoid unsound cast; guard before emitting API error
Apply: - eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError });
+ const apiErr = isClerkAPIResponseError(err) ? err : null;
+ eventBus.emit('resource:error', { resource: this.resource, error: apiErr });Add this helper (once per module or in a shared util): // Type guard for API errors
function isClerkAPIResponseError(err: unknown): err is ClerkAPIResponseError {
return !!err && typeof err === 'object' && Array.isArray((err as any).errors);
}🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -572,7 +573,7 @@ class SignInFuture implements SignInFutureResource { | |||||||||||||||||||||
| action: 'attempt_first_factor', | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
| } catch (err: unknown) { | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError }); | ||||||||||||||||||||||
| return { error: err }; | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
575
to
577
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same guard needed here Don’t cast arbitrary errors to - eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError });
+ const apiErr = isClerkAPIResponseError(err) ? err : null;
+ eventBus.emit('resource:error', { resource: this.resource, error: apiErr });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
@@ -612,7 +613,7 @@ class SignInFuture implements SignInFutureResource { | |||||||||||||||||||||
| windowNavigate(externalVerificationRedirectURL); | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
| } catch (err: unknown) { | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err }); | ||||||||||||||||||||||
| eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError }); | ||||||||||||||||||||||
| return { error: err }; | ||||||||||||||||||||||
|
Comment on lines
615
to
616
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Guard before emitting in SSO flow This path throws local errors (e.g., “modal flow is not supported yet”). Use the same guard to avoid mis-typing the signal. - eventBus.emit('resource:error', { resource: this.resource, error: err as ClerkAPIResponseError });
+ const apiErr = isClerkAPIResponseError(err) ? err : null;
+ eventBus.emit('resource:error', { resource: this.resource, error: apiErr });📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,80 @@ | ||
| import type { ClerkAPIResponseError, Errors } from '@clerk/types'; | ||
| import { computed, signal } from 'alien-signals'; | ||
|
|
||
| import type { SignIn } from './resources/SignIn'; | ||
|
|
||
| export const signInSignal = signal<{ resource: SignIn | null }>({ resource: null }); | ||
| export const signInErrorSignal = signal<{ errors: unknown }>({ errors: null }); | ||
| export const signInErrorSignal = signal<{ error: ClerkAPIResponseError | null }>({ error: null }); | ||
|
|
||
| export const signInComputedSignal = computed(() => { | ||
| const signIn = signInSignal().resource; | ||
| const errors = signInErrorSignal().errors; | ||
| const error = signInErrorSignal().error; | ||
|
|
||
| const errors = errorsToParsedErrors(error); | ||
|
|
||
| if (!signIn) { | ||
| return { errors: null, signIn: null }; | ||
| return { errors, signIn: null }; | ||
| } | ||
|
|
||
| return { errors, signIn: signIn.__internal_future }; | ||
| }); | ||
|
|
||
| export function errorsToParsedErrors(error: ClerkAPIResponseError | null): Errors { | ||
| const parsedErrors: Errors = { | ||
| fields: { | ||
| firstName: null, | ||
| lastName: null, | ||
| emailAddress: null, | ||
| identifier: null, | ||
| phoneNumber: null, | ||
| password: null, | ||
| username: null, | ||
| code: null, | ||
| captcha: null, | ||
| legalAccepted: null, | ||
| }, | ||
| raw: error ? error.errors : [], | ||
| global: [], | ||
| }; | ||
|
|
||
| error?.errors.forEach(error => { | ||
| if ('meta' in error && error.meta && 'paramName' in error.meta) { | ||
| switch (error.meta.paramName) { | ||
| case 'first_name': | ||
| parsedErrors.fields.firstName = error; | ||
| break; | ||
| case 'last_name': | ||
| parsedErrors.fields.lastName = error; | ||
| break; | ||
| case 'email_address': | ||
| parsedErrors.fields.emailAddress = error; | ||
| break; | ||
| case 'identifier': | ||
| parsedErrors.fields.identifier = error; | ||
| break; | ||
| case 'phone_number': | ||
| parsedErrors.fields.phoneNumber = error; | ||
| break; | ||
| case 'password': | ||
| parsedErrors.fields.password = error; | ||
| break; | ||
| case 'username': | ||
| parsedErrors.fields.username = error; | ||
| break; | ||
| case 'code': | ||
| parsedErrors.fields.code = error; | ||
| break; | ||
| case 'captcha': | ||
| parsedErrors.fields.captcha = error; | ||
| break; | ||
| case 'legal_accepted': | ||
| parsedErrors.fields.legalAccepted = error; | ||
| break; | ||
| } | ||
|
Comment on lines
+53
to
+89
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Field errors may be overwritten when multiple errors exist for the same field The current implementation assigns errors directly to field properties (e.g., Consider either:
Example of potential data loss: // If API returns two errors for 'first_name':
// Error 1: "First name is required"
// Error 2: "First name contains invalid characters"
// Only Error 2 would be retained in parsedErrors.fields.firstName🤖 Prompt for AI Agents |
||
| } else { | ||
| parsedErrors.global.push(error); | ||
| } | ||
| }); | ||
|
coderabbitai[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| return parsedErrors; | ||
| } | ||
|
Comment on lines
+27
to
+96
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add unit tests for the new error parsing functionality The
🤖 Prompt for AI Agents |
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,10 +1,9 @@ | ||||||||||||||||||||||||||||
| import type { SignInFutureResource } from '@clerk/types'; | ||||||||||||||||||||||||||||
| import { useCallback, useSyncExternalStore } from 'react'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import { useIsomorphicClerkContext } from '../contexts/IsomorphicClerkContext'; | ||||||||||||||||||||||||||||
| import { useAssertWrappedByClerkProvider } from './useAssertWrappedByClerkProvider'; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| function useClerkSignal(signal: 'signIn'): { errors: unknown; signIn: SignInFutureResource | null } | null { | ||||||||||||||||||||||||||||
| function useClerkSignal(signal: 'signIn') { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Restore explicit return types for public APIs Per guidelines, exported hooks should have explicit return types. You can capture the core’s type without duplicating shapes via +import type { State as ClerkState } from '@clerk/types';
-function useClerkSignal(signal: 'signIn') {
+function useClerkSignal(signal: 'signIn'): ReturnType<ClerkState['signInSignal']> | null {
…
}
-export function useSignInSignal() {
+export function useSignInSignal(): ReturnType<ClerkState['signInSignal']> | null {
return useClerkSignal('signIn');
}📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||
| useAssertWrappedByClerkProvider('useSignInSignal'); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| const clerk = useIsomorphicClerkContext(); | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
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.
💡 Verification agent
🧩 Analysis chain
Verify all emitters conform to the new payload
Now that
ResourceErrorPayload.errorisClerkAPIResponseError | null, ensure alleventBus.emit('resource:error', …)sites either pass a properly typed API error ornull(for non-API errors).Run this to find non-conforming emitters:
🏁 Script executed:
Length of output: 5359
Ensure all
resource:erroremits useClerkAPIResponseError | nullA couple of
eventBus.emit('resource:error', …)calls are still passing the rawerr(typeunknown) instead of casting it toClerkAPIResponseError. Please update them to match the new payload type:• packages/clerk-js/src/core/resources/SignIn.ts
Change each instance from:
to:
This ensures
ResourceErrorPayload.erroris alwaysClerkAPIResponseErrorornull.🤖 Prompt for AI Agents