diff --git a/.changeset/cold-trees-hammer.md b/.changeset/cold-trees-hammer.md new file mode 100644 index 00000000000..48dff84abcb --- /dev/null +++ b/.changeset/cold-trees-hammer.md @@ -0,0 +1,5 @@ +--- +'@clerk/shared': minor +--- + +Add timeout-based mechanism to detect when clerk-js fails to load and set the status to `error` on isomorphicClerk \ No newline at end of file diff --git a/integration/tests/resiliency.test.ts b/integration/tests/resiliency.test.ts index 30efae92ba8..b4d2505ae50 100644 --- a/integration/tests/resiliency.test.ts +++ b/integration/tests/resiliency.test.ts @@ -184,8 +184,9 @@ testAgainstRunningApps({ withEnv: [appConfigs.envs.withEmailCodes] })('resilienc await expect(page.getByText('Clerk is NOT loaded', { exact: true })).toBeVisible(); // Wait for loading to complete and verify final state + // Account for the new 15-second script loading timeout plus buffer for UI updates await expect(page.getByText('Status: error', { exact: true })).toBeVisible({ - timeout: 10_000, + timeout: 16_000, }); await expect(page.getByText('Clerk is out', { exact: true })).toBeVisible(); await expect(page.getByText('Clerk is ready or degraded (loaded)')).toBeHidden(); diff --git a/packages/shared/src/__tests__/loadClerkJsScript.test.ts b/packages/shared/src/__tests__/loadClerkJsScript.test.ts index 2e259e10292..bc8b58824ae 100644 --- a/packages/shared/src/__tests__/loadClerkJsScript.test.ts +++ b/packages/shared/src/__tests__/loadClerkJsScript.test.ts @@ -12,6 +12,12 @@ jest.mock('../loadScript'); setClerkJsLoadingErrorPackageName('@clerk/clerk-react'); const jsPackageMajorVersion = getMajorVersion(JS_PACKAGE_VERSION); +const mockClerk = { + status: 'ready', + loaded: true, + load: jest.fn(), +}; + describe('loadClerkJsScript(options)', () => { const mockPublishableKey = 'pk_test_Zm9vLWJhci0xMy5jbGVyay5hY2NvdW50cy5kZXYk'; @@ -19,17 +25,43 @@ describe('loadClerkJsScript(options)', () => { jest.clearAllMocks(); (loadScript as jest.Mock).mockResolvedValue(undefined); document.querySelector = jest.fn().mockReturnValue(null); + + (window as any).Clerk = undefined; + + jest.useFakeTimers(); + }); + + afterEach(() => { + jest.useRealTimers(); }); test('throws error when publishableKey is missing', async () => { - await expect(() => loadClerkJsScript({} as any)).rejects.toThrow( + await expect(loadClerkJsScript({} as any)).rejects.toThrow( '@clerk/clerk-react: Missing publishableKey. You can get your key at https://dashboard.clerk.com/last-active?path=api-keys.', ); }); - test('loads script when no existing script is found', async () => { - await loadClerkJsScript({ publishableKey: mockPublishableKey }); + test('returns null immediately when Clerk is already loaded', async () => { + (window as any).Clerk = mockClerk; + const result = await loadClerkJsScript({ publishableKey: mockPublishableKey }); + expect(result).toBeNull(); + expect(loadScript).not.toHaveBeenCalled(); + }); + + test('loads script and waits for Clerk to be available', async () => { + const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey }); + + // Simulate Clerk becoming available after 250ms + setTimeout(() => { + (window as any).Clerk = mockClerk; + }, 250); + + // Advance timers to allow polling to detect Clerk + jest.advanceTimersByTime(300); + + const result = await loadPromise; + expect(result).toBeNull(); expect(loadScript).toHaveBeenCalledWith( expect.stringContaining( `https://foo-bar-13.clerk.accounts.dev/npm/@clerk/clerk-js@${jsPackageMajorVersion}/dist/clerk.browser.js`, @@ -42,34 +74,74 @@ describe('loadClerkJsScript(options)', () => { ); }); - test('uses existing script when found', async () => { - const mockExistingScript = document.createElement('script'); - document.querySelector = jest.fn().mockReturnValue(mockExistingScript); + test('times out and rejects when Clerk does not load', async () => { + let rejectedWith: any; - const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey }); - mockExistingScript.dispatchEvent(new Event('load')); + const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey, scriptLoadTimeout: 1000 }); - await expect(loadPromise).resolves.toBe(mockExistingScript); - expect(loadScript).not.toHaveBeenCalled(); + try { + jest.advanceTimersByTime(1000); + await loadPromise; + } catch (error) { + rejectedWith = error; + } + + expect(rejectedWith).toBeInstanceOf(Error); + expect(rejectedWith.message).toBe('Clerk: Failed to load Clerk'); + expect((window as any).Clerk).toBeUndefined(); }); - test('rejects when existing script fails to load', async () => { + test('waits for existing script with timeout', async () => { const mockExistingScript = document.createElement('script'); document.querySelector = jest.fn().mockReturnValue(mockExistingScript); const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey }); - mockExistingScript.dispatchEvent(new Event('error')); - await expect(loadPromise).rejects.toBe('Clerk: Failed to load Clerk'); + // Simulate Clerk becoming available after 250ms + setTimeout(() => { + (window as any).Clerk = mockClerk; + }, 250); + + // Advance timers to allow polling to detect Clerk + jest.advanceTimersByTime(300); + + const result = await loadPromise; + expect(result).toBeNull(); expect(loadScript).not.toHaveBeenCalled(); }); - test('throws error when loadScript fails', async () => { - (loadScript as jest.Mock).mockRejectedValue(new Error('Script load failed')); + test('handles race condition when Clerk loads just as timeout fires', async () => { + const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey, scriptLoadTimeout: 1000 }); - await expect(loadClerkJsScript({ publishableKey: mockPublishableKey })).rejects.toThrow( - 'Clerk: Failed to load Clerk', - ); + setTimeout(() => { + (window as any).Clerk = mockClerk; + }, 999); + + jest.advanceTimersByTime(1000); + + const result = await loadPromise; + expect(result).toBeNull(); + expect((window as any).Clerk).toBe(mockClerk); + }); + + test('validates Clerk is properly loaded with required methods', async () => { + const loadPromise = loadClerkJsScript({ publishableKey: mockPublishableKey }); + + setTimeout(() => { + (window as any).Clerk = { status: 'ready' }; + }, 100); + + jest.advanceTimersByTime(15000); + + try { + await loadPromise; + fail('Should have thrown error'); + } catch (error) { + expect(error).toBeInstanceOf(Error); + expect((error as Error).message).toBe('Clerk: Failed to load Clerk'); + // The malformed Clerk object should still be there since it was set + expect((window as any).Clerk).toEqual({ status: 'ready' }); + } }); }); diff --git a/packages/shared/src/loadClerkJsScript.ts b/packages/shared/src/loadClerkJsScript.ts index 97647e3f0bd..f9edff97eb4 100644 --- a/packages/shared/src/loadClerkJsScript.ts +++ b/packages/shared/src/loadClerkJsScript.ts @@ -16,8 +16,11 @@ const errorThrower = buildErrorThrower({ packageName: '@clerk/shared' }); /** * Sets the package name for error messages during ClerkJS script loading. * + * @param packageName - The name of the package to use in error messages (e.g., '@clerk/clerk-react'). * @example + * ```typescript * setClerkJsLoadingErrorPackageName('@clerk/clerk-react'); + * ``` */ export function setClerkJsLoadingErrorPackageName(packageName: string) { errorThrower.setPackageName({ packageName }); @@ -32,42 +35,125 @@ type LoadClerkJsScriptOptions = Without & { proxyUrl?: string; domain?: string; nonce?: string; + /** + * Timeout in milliseconds to wait for clerk-js to load before considering it failed. + * + * @default 15000 (15 seconds) + */ + scriptLoadTimeout?: number; }; /** - * Hotloads the Clerk JS script. + * Validates that window.Clerk exists and is properly initialized. + * This ensures we don't have false positives where the script loads but Clerk is malformed. * - * Checks for an existing Clerk JS script. If found, it returns a promise - * that resolves when the script loads. If not found, it uses the provided options to - * build the Clerk JS script URL and load the script. + * @returns `true` if window.Clerk exists and has the expected structure with a load method. + */ +function isClerkProperlyLoaded(): boolean { + if (typeof window === 'undefined' || !(window as any).Clerk) { + return false; + } + + // Basic validation that window.Clerk has the expected structure + const clerk = (window as any).Clerk; + return typeof clerk === 'object' && typeof clerk.load === 'function'; +} + +/** + * Waits for Clerk to be properly loaded with a timeout mechanism. + * Uses polling to check if Clerk becomes available within the specified timeout. + * + * @param timeoutMs - Maximum time to wait in milliseconds. + * @returns Promise that resolves with null if Clerk loads successfully, or rejects with an error if timeout is reached. + */ +function waitForClerkWithTimeout(timeoutMs: number): Promise { + return new Promise((resolve, reject) => { + let resolved = false; + + const cleanup = (timeoutId: ReturnType, pollInterval: ReturnType) => { + clearTimeout(timeoutId); + clearInterval(pollInterval); + }; + + const checkAndResolve = () => { + if (resolved) return; + + if (isClerkProperlyLoaded()) { + resolved = true; + cleanup(timeoutId, pollInterval); + resolve(null); + } + }; + + const handleTimeout = () => { + if (resolved) return; + + resolved = true; + cleanup(timeoutId, pollInterval); + + if (!isClerkProperlyLoaded()) { + reject(new Error(FAILED_TO_LOAD_ERROR)); + } else { + resolve(null); + } + }; + + const timeoutId = setTimeout(handleTimeout, timeoutMs); + + checkAndResolve(); + + const pollInterval = setInterval(() => { + if (resolved) { + clearInterval(pollInterval); + return; + } + checkAndResolve(); + }, 100); + }); +} + +/** + * Hotloads the Clerk JS script with robust failure detection. + * + * Uses a timeout-based approach to ensure absolute certainty about load success/failure. + * If the script fails to load within the timeout period, or loads but doesn't create + * a proper Clerk instance, the promise rejects with an error. * * @param opts - The options used to build the Clerk JS script URL and load the script. * Must include a `publishableKey` if no existing script is found. + * @returns Promise that resolves with null if Clerk loads successfully, or rejects with an error. * * @example - * loadClerkJsScript({ publishableKey: 'pk_' }); + * ```typescript + * try { + * await loadClerkJsScript({ publishableKey: 'pk_test_...' }); + * console.log('Clerk loaded successfully'); + * } catch (error) { + * console.error('Failed to load Clerk:', error.message); + * } + * ``` */ -const loadClerkJsScript = async (opts?: LoadClerkJsScriptOptions) => { +const loadClerkJsScript = async (opts?: LoadClerkJsScriptOptions): Promise => { + const timeout = opts?.scriptLoadTimeout ?? 15000; + + if (isClerkProperlyLoaded()) { + return null; + } + const existingScript = document.querySelector('script[data-clerk-js-script]'); if (existingScript) { - return new Promise((resolve, reject) => { - existingScript.addEventListener('load', () => { - resolve(existingScript); - }); - - existingScript.addEventListener('error', () => { - reject(FAILED_TO_LOAD_ERROR); - }); - }); + return waitForClerkWithTimeout(timeout); } if (!opts?.publishableKey) { errorThrower.throwMissingPublishableKeyError(); - return; + return null; } - return loadScript(clerkJsScriptUrl(opts), { + const loadPromise = waitForClerkWithTimeout(timeout); + + loadScript(clerkJsScriptUrl(opts), { async: true, crossOrigin: 'anonymous', nonce: opts.nonce, @@ -75,15 +161,21 @@ const loadClerkJsScript = async (opts?: LoadClerkJsScriptOptions) => { }).catch(() => { throw new Error(FAILED_TO_LOAD_ERROR); }); + + return loadPromise; }; /** - * Generates a Clerk JS script URL. + * Generates a Clerk JS script URL based on the provided options. * * @param opts - The options to use when building the Clerk JS script URL. + * @returns The complete URL to the Clerk JS script. * * @example - * clerkJsScriptUrl({ publishableKey: 'pk_' }); + * ```typescript + * const url = clerkJsScriptUrl({ publishableKey: 'pk_test_...' }); + * // Returns: "https://example.clerk.accounts.dev/npm/@clerk/clerk-js@5/dist/clerk.browser.js" + * ``` */ const clerkJsScriptUrl = (opts: LoadClerkJsScriptOptions) => { const { clerkJSUrl, clerkJSVariant, clerkJSVersion, proxyUrl, domain, publishableKey } = opts; @@ -107,7 +199,10 @@ const clerkJsScriptUrl = (opts: LoadClerkJsScriptOptions) => { }; /** - * Builds an object of Clerk JS script attributes. + * Builds an object of Clerk JS script attributes based on the provided options. + * + * @param options - The options containing the values for script attributes. + * @returns An object containing data attributes to be applied to the script element. */ const buildClerkJsScriptAttributes = (options: LoadClerkJsScriptOptions) => { const obj: Record = {}; @@ -131,6 +226,12 @@ const buildClerkJsScriptAttributes = (options: LoadClerkJsScriptOptions) => { return obj; }; +/** + * Returns a function that applies Clerk JS script attributes to a script element. + * + * @param options - The options containing the values for script attributes. + * @returns A function that accepts a script element and applies the attributes to it. + */ const applyClerkJsScriptAttributes = (options: LoadClerkJsScriptOptions) => (script: HTMLScriptElement) => { const attributes = buildClerkJsScriptAttributes(options); for (const attribute in attributes) { diff --git a/packages/shared/src/telemetry/collector.ts b/packages/shared/src/telemetry/collector.ts index a9af381f151..ba99f92b824 100644 --- a/packages/shared/src/telemetry/collector.ts +++ b/packages/shared/src/telemetry/collector.ts @@ -1,6 +1,6 @@ /** * The `TelemetryCollector` class handles collection of telemetry events from Clerk SDKs. Telemetry is opt-out and can be disabled by setting a CLERK_TELEMETRY_DISABLED environment variable. - * The `ClerkProvider` also accepts a `telemetry` prop that will be passed to the collector during initialization: + * The `ClerkProvider` also accepts a `telemetry` prop that will be passed to the collector during initialization:. * * ```jsx * @@ -8,10 +8,11 @@ * * ``` * - * For more information, please see the telemetry documentation page: https://clerk.com/docs/telemetry + * For more information, please see the telemetry documentation page: https://clerk.com/docs/telemetry. */ import type { InstanceType, + SDKMetadata, TelemetryCollector as TelemetryCollectorInterface, TelemetryEvent, TelemetryEventRaw, @@ -22,6 +23,31 @@ import { isTruthy } from '../underscore'; import { TelemetryEventThrottler } from './throttler'; import type { TelemetryCollectorOptions } from './types'; +/** + * Local interface for window.Clerk to avoid global type pollution. + * This is only used within this module and doesn't affect other packages. + */ +interface WindowWithClerk extends Window { + Clerk?: { + constructor?: { + sdkMetadata?: SDKMetadata; + }; + }; +} + +/** + * Type guard to check if window.Clerk exists and has the expected structure. + */ +function isWindowClerkWithMetadata(clerk: unknown): clerk is { constructor: { sdkMetadata?: SDKMetadata } } { + return ( + typeof clerk === 'object' && + clerk !== null && + 'constructor' in clerk && + typeof clerk.constructor === 'object' && + clerk.constructor !== null + ); +} + type TelemetryCollectorConfig = Pick< TelemetryCollectorOptions, 'samplingRate' | 'disabled' | 'debug' | 'maxBufferSize' @@ -231,15 +257,29 @@ export class TelemetryCollector implements TelemetryCollectorInterface { * This is necessary because the sdkMetadata can be set by the host SDK after the TelemetryCollector is instantiated. */ #getSDKMetadata() { - let sdkMetadata = { + const sdkMetadata = { name: this.#metadata.sdk, version: this.#metadata.sdkVersion, }; - // @ts-expect-error -- The global window.Clerk type is declared in clerk-js, but we can't rely on that here - if (typeof window !== 'undefined' && window.Clerk) { - // @ts-expect-error -- The global window.Clerk type is declared in clerk-js, but we can't rely on that here - sdkMetadata = { ...sdkMetadata, ...window.Clerk.constructor.sdkMetadata }; + if (typeof window !== 'undefined') { + const windowWithClerk = window as WindowWithClerk; + + if (windowWithClerk.Clerk) { + const windowClerk = windowWithClerk.Clerk; + + if (isWindowClerkWithMetadata(windowClerk) && windowClerk.constructor.sdkMetadata) { + const { name, version } = windowClerk.constructor.sdkMetadata; + + // Only update properties if they exist to avoid overwriting with undefined + if (name !== undefined) { + sdkMetadata.name = name; + } + if (version !== undefined) { + sdkMetadata.version = version; + } + } + } } return sdkMetadata;