Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 469
fix(expo): Load SSO deps via require to fix Metro async-chunk failure#8720
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
b13e461e68a9e81c41b927f77e1bFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| '@clerk/expo': patch | ||
| --- | ||
| Fix `useSSO()` in Expo apps that hit module loading failures when starting an SSO flow under Metro. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,99 @@ | ||
| import { renderHook } from '@testing-library/react'; | ||
| import { afterEach, beforeEach, describe, expect, test, vi } from 'vitest'; | ||
| import { useSSO } from '../useSSO'; | ||
| const mocks = vi.hoisted(() => { | ||
| return { | ||
| useSignIn: vi.fn(), | ||
| useSignUp: vi.fn(), | ||
| openAuthSessionAsync: vi.fn(), | ||
| }; | ||
| }); | ||
| vi.mock('@clerk/react/legacy', () => { | ||
| return { | ||
| useSignIn: mocks.useSignIn, | ||
| useSignUp: mocks.useSignUp, | ||
| }; | ||
| }); | ||
| vi.mock('react-native', () => { | ||
| return { | ||
| Platform: { | ||
| OS: 'ios', | ||
| }, | ||
| }; | ||
| }); | ||
| vi.mock('expo-web-browser', () => { | ||
| return { | ||
| openAuthSessionAsync: mocks.openAuthSessionAsync, | ||
| }; | ||
| }); | ||
| // expo-auth-session is intentionally left unmocked: it cannot be require()'d in this environment, | ||
| // which exercises the dependency-load failure path (the bug behind #8288). Only the error-path | ||
| // test reaches that require(); the other tests return before it, so they are unaffected. | ||
| describe('useSSO', () => { | ||
| const mockSignIn = { | ||
| create: vi.fn(), | ||
| }; | ||
| const mockSignUp = { | ||
| create: vi.fn(), | ||
| createdSessionId: null, | ||
| }; | ||
| const mockSetActive = vi.fn(); | ||
| beforeEach(() => { | ||
| vi.clearAllMocks(); | ||
| mocks.useSignIn.mockReturnValue({ | ||
| signIn: mockSignIn, | ||
| setActive: mockSetActive, | ||
| isLoaded: true, | ||
| }); | ||
| mocks.useSignUp.mockReturnValue({ | ||
| signUp: mockSignUp, | ||
| isLoaded: true, | ||
| }); | ||
| }); | ||
| afterEach(() => { | ||
| vi.restoreAllMocks(); | ||
| }); | ||
| test('returns the startSSOFlow function', () => { | ||
| const { result } = renderHook(() => useSSO()); | ||
| expect(typeof result.current.startSSOFlow).toBe('function'); | ||
| }); | ||
| test('returns early without starting the flow when Clerk is not loaded', async () => { | ||
| mocks.useSignIn.mockReturnValue({ | ||
| signIn: mockSignIn, | ||
| setActive: mockSetActive, | ||
| isLoaded: false, | ||
| }); | ||
| const { result } = renderHook(() => useSSO()); | ||
| const response = await result.current.startSSOFlow({ strategy: 'oauth_google' }); | ||
| expect(mockSignIn.create).not.toHaveBeenCalled(); | ||
| expect(mocks.openAuthSessionAsync).not.toHaveBeenCalled(); | ||
| expect(response.createdSessionId).toBe(null); | ||
| }); | ||
| test('surfaces the underlying error when an auth-session dependency fails to load', async () => { | ||
| const { result } = renderHook(() => useSSO()); | ||
| await expect(result.current.startSSOFlow({ strategy: 'oauth_google' })).rejects.toThrow( | ||
| /required for SSO: .+\. If they are not installed/s, | ||
| ); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -47,17 +47,22 @@ export function useSSO() { | ||
| }; | ||
| } | ||
| // Dynamically import expo-auth-session and expo-web-browser only when needed | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- dynamic import of optional dependency | ||
| // Load via synchronous require() instead of import(): Metro inlines require() into the main | ||
| // bundle, while import() emits an async chunk that fails to resolve without @expo/metro-runtime. | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- type-only annotation for optional dependency | ||
| let AuthSession: typeof import('expo-auth-session'); | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- dynamic import of optional dependency | ||
| // eslint-disable-next-line @typescript-eslint/consistent-type-imports -- type-only annotation for optional dependency | ||
| let WebBrowserModule: typeof import('expo-web-browser'); | ||
| try { | ||
| [AuthSession, WebBrowserModule] = await Promise.all([import('expo-auth-session'), import('expo-web-browser')]); | ||
| } catch { | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| AuthSession = require('expo-auth-session'); | ||
| // eslint-disable-next-line @typescript-eslint/no-require-imports | ||
| WebBrowserModule = require('expo-web-browser'); | ||
| } catch (err) { | ||
| return errorThrower.throw( | ||
| 'expo-auth-session and expo-web-browser are required for SSO. ' + | ||
| 'Install them: npx expo install expo-auth-session expo-web-browser', | ||
| `Unable to load expo-auth-session and expo-web-browser, which are required for SSO: ${ | ||
| err instanceof Error ? err.message : 'Unknown error' | ||
| }. If they are not installed, run: npx expo install expo-auth-session expo-web-browser`, | ||
Comment on lines
+63
to
+65
MemberAuthor 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. Keeps the install hint but no longer hides the real error | ||
| ); | ||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.
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.
these dont test the actual fix,
import()vsrequire()is Metro resolution behavior that Vitest/Node can't reproduce but the last test does pin down the behavior change we care about