From c2ead9b973d9a7864c6aea22ed45655ee84d4b81 Mon Sep 17 00:00:00 2001 From: Ryan Bas Date: Tue, 20 Jan 2026 15:49:41 -0700 Subject: [PATCH] fix!: update-error-handling-across-packages --- .changeset/chilly-sheep-follow.md | 40 ++++++++++++++++ .../src/lib/client.store.test.ts | 31 ++++++++++--- .../journey-client/src/lib/client.store.ts | 46 +++++++++++++++---- .../src/lib/device/device-profile.test.ts | 4 +- .../journey-client/src/lib/journey.utils.ts | 18 ++++++-- 5 files changed, 119 insertions(+), 20 deletions(-) create mode 100644 .changeset/chilly-sheep-follow.md diff --git a/.changeset/chilly-sheep-follow.md b/.changeset/chilly-sheep-follow.md new file mode 100644 index 000000000..a00728d67 --- /dev/null +++ b/.changeset/chilly-sheep-follow.md @@ -0,0 +1,40 @@ +--- +'@forgerock/storage': major +'@forgerock/sdk-oidc': major +'@forgerock/journey-client': major +'@forgerock/oidc-client': major +--- + +**BREAKING CHANGE**: Journey client methods now return `GenericError` instead of `undefined` for error cases. + +## What Changed + +The `start`, `next`, `resume`, and `terminate` methods now return a `GenericError` object instead of `undefined` when encountering unknown step types or error conditions. This aligns the journey client with the DaVinci client's error handling patterns. + +### Return Type Changes + +| Method | Before | After | +| ----------- | ------------------------------------------------------------------------ | --------------------------------------------------------------------------- | +| `start` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` | +| `next` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` | +| `resume` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` | +| `terminate` | `void` | `void \| GenericError` | + +## Migration Guide + +Before: + +```ts +const step = await client.start(); +if (step) { + // Use step +} + +After: const result = await client.start(); +if ('error' in result) { + // Handle error + console.error(result.message); +} else { + // Use step +} +``` diff --git a/packages/journey-client/src/lib/client.store.test.ts b/packages/journey-client/src/lib/client.store.test.ts index dfd8b3151..8bb662f9f 100644 --- a/packages/journey-client/src/lib/client.store.test.ts +++ b/packages/journey-client/src/lib/client.store.test.ts @@ -8,12 +8,19 @@ import { callbackType } from '@forgerock/sdk-types'; import { afterEach, describe, expect, test, vi } from 'vitest'; -import type { Step } from '@forgerock/sdk-types'; +import type { GenericError, Step } from '@forgerock/sdk-types'; import { journey } from './client.store.js'; import { createJourneyStep } from './step.utils.js'; import { JourneyClientConfig } from './config.types.js'; +/** + * Type guard to check if a result is a GenericError + */ +function isGenericError(result: unknown): result is GenericError { + return typeof result === 'object' && result !== null && 'error' in result && 'type' in result; +} + // Create a singleton mock instance for storage const mockStorageInstance = { get: vi.fn(), @@ -65,13 +72,16 @@ describe('journey-client', () => { const client = await journey({ config: mockConfig }); const step = await client.start(); expect(step).toBeDefined(); + expect(isGenericError(step)).toBe(false); expect(mockFetch).toHaveBeenCalledTimes(1); const request = mockFetch.mock.calls[0][0] as Request; // TODO: This should be /journeys?_action=start, but the current implementation calls /authenticate expect(request.url).toBe('https://test.com/json/realms/root/authenticate'); expect(step).toHaveProperty('type', 'Step'); - expect(step && step.payload).toEqual(mockStepResponse); + if (!isGenericError(step)) { + expect(step.payload).toEqual(mockStepResponse); + } }); test('next() should send the current step and return the next step', async () => { @@ -101,6 +111,7 @@ describe('journey-client', () => { const client = await journey({ config: mockConfig }); const nextStep = await client.next(initialStep, {}); expect(nextStep).toBeDefined(); + expect(isGenericError(nextStep)).toBe(false); expect(mockFetch).toHaveBeenCalledTimes(1); const request = mockFetch.mock.calls[0][0] as Request; @@ -109,7 +120,9 @@ describe('journey-client', () => { expect(request.method).toBe('POST'); expect(await request.json()).toEqual(initialStep.payload); expect(nextStep).toHaveProperty('type', 'Step'); - expect(nextStep && nextStep.payload).toEqual(nextStepPayload); + if (!isGenericError(nextStep)) { + expect(nextStep.payload).toEqual(nextStepPayload); + } }); test('redirect() should store the step and call location.assign', async () => { @@ -169,7 +182,9 @@ describe('journey-client', () => { expect(request.method).toBe('POST'); expect(await request.json()).toEqual(previousStepPayload); expect(step).toHaveProperty('type', 'Step'); - expect(step && step.payload).toEqual(nextStepPayload); + if (!isGenericError(step)) { + expect(step.payload).toEqual(nextStepPayload); + } }); test('should correctly resume with a plain Step object from storage', async () => { @@ -198,7 +213,9 @@ describe('journey-client', () => { expect(request.method).toBe('POST'); expect(await request.json()).toEqual(plainStepPayload); // Expect the plain payload to be sent expect(step).toHaveProperty('type', 'Step'); // The returned step should still be an JourneyStep instance - expect(step && step.payload).toEqual(nextStepPayload); + if (!isGenericError(step)) { + expect(step.payload).toEqual(nextStepPayload); + } }); test('should throw an error if a previous step is required but not found', async () => { @@ -234,7 +251,9 @@ describe('journey-client', () => { const url = new URL(request.url); expect(url.origin + url.pathname).toBe('https://test.com/json/realms/root/authenticate'); expect(step).toHaveProperty('type', 'Step'); - expect(step && step.payload).toEqual(mockStepResponse); + if (!isGenericError(step)) { + expect(step.payload).toEqual(mockStepResponse); + } }); }); diff --git a/packages/journey-client/src/lib/client.store.ts b/packages/journey-client/src/lib/client.store.ts index 9288cd0dc..6c2ca44d8 100644 --- a/packages/journey-client/src/lib/client.store.ts +++ b/packages/journey-client/src/lib/client.store.ts @@ -70,19 +70,36 @@ export async function journey({ const self = { start: async (options?: StartParam) => { const { data } = await store.dispatch(journeyApi.endpoints.start.initiate(options)); - return data ? createJourneyObject(data) : undefined; + if (!data) { + const error: GenericError = { + error: 'no_response_data', + message: 'No data received from server when starting journey', + type: 'unknown_error', + }; + return error; + } + return createJourneyObject(data); }, /** * Submits the current Step payload to the authentication API and retrieves the next JourneyStep in the journey. * The `step` to be submitted is provided within the `options` object. * - * @param options An object containing the current Step payload and optional JourneyClientConfig. - * @returns A Promise that resolves to the next JourneyStep in the journey, or undefined if the journey ends. + * @param step The current JourneyStep containing user input to submit. + * @param options Optional configuration for the request. + * @returns A Promise that resolves to a JourneyStep, JourneyLoginSuccess, JourneyLoginFailure, or GenericError. */ next: async (step: JourneyStep, options?: NextOptions) => { const { data } = await store.dispatch(journeyApi.endpoints.next.initiate({ step, options })); - return data ? createJourneyObject(data) : undefined; + if (!data) { + const error: GenericError = { + error: 'no_response_data', + message: 'No data received from server when submitting step', + type: 'unknown_error', + }; + return error; + } + return createJourneyObject(data); }, // TODO: Remove the actual redirect from this method and just return the URL to the caller @@ -108,7 +125,7 @@ export async function journey({ resume: async ( url: string, options?: ResumeOptions, - ): Promise | undefined> => { + ): Promise> => { const parsedUrl = new URL(url); const code = parsedUrl.searchParams.get('code'); const state = parsedUrl.searchParams.get('state'); @@ -183,11 +200,22 @@ export async function journey({ * This will invalidate the current session and clean up any server-side state. * * @param options Optional StepOptions containing query parameters. - * @returns A Promise that resolves when the session is successfully ended. + * @returns A Promise that resolves to void on success, or GenericError on failure. */ - terminate: async (options?: { query?: Record }) => { - const { data } = await store.dispatch(journeyApi.endpoints.terminate.initiate(options)); - return data; + terminate: async (options?: { + query?: Record; + }): Promise => { + const { error } = await store.dispatch(journeyApi.endpoints.terminate.initiate(options)); + if (error) { + return { + error: 'terminate_failed', + message: + 'status' in error + ? `Failed to terminate session: ${error.status}` + : 'Failed to terminate session', + type: 'unknown_error', + }; + } }, }; return self; diff --git a/packages/journey-client/src/lib/device/device-profile.test.ts b/packages/journey-client/src/lib/device/device-profile.test.ts index bf9296e14..2c57a685b 100644 --- a/packages/journey-client/src/lib/device/device-profile.test.ts +++ b/packages/journey-client/src/lib/device/device-profile.test.ts @@ -7,7 +7,7 @@ * This software may be modified and distributed under the terms * of the MIT license. See the LICENSE file for details. */ -import { vi, expect, describe, it, afterEach, beforeEach, SpyInstance } from 'vitest'; +import { vi, expect, describe, it, afterEach, beforeEach, MockInstance } from 'vitest'; import { Device } from './device-profile.js'; @@ -86,7 +86,7 @@ describe('Test DeviceProfile', () => { }); describe('logLevel tests', () => { - let warnSpy: SpyInstance; + let warnSpy: MockInstance; const originalNavigator = global.navigator; beforeEach(() => { diff --git a/packages/journey-client/src/lib/journey.utils.ts b/packages/journey-client/src/lib/journey.utils.ts index d5efc38e3..4a42cae87 100644 --- a/packages/journey-client/src/lib/journey.utils.ts +++ b/packages/journey-client/src/lib/journey.utils.ts @@ -7,7 +7,7 @@ import { StepType } from '@forgerock/sdk-types'; -import type { Step } from '@forgerock/sdk-types'; +import type { GenericError, Step } from '@forgerock/sdk-types'; import { createJourneyLoginSuccess } from './login-success.utils.js'; import { createJourneyLoginFailure } from './login-failure.utils.js'; @@ -17,9 +17,17 @@ import type { JourneyStep } from './step.utils.js'; import type { JourneyLoginFailure } from './login-failure.utils.js'; import type { JourneyLoginSuccess } from './login-success.utils.js'; +/** + * Creates a journey object from a raw Step response. + * Determines the step type based on the presence of authId or successUrl properties + * and returns the appropriate journey object type. + * + * @param step - The raw Step response from the authentication API + * @returns A JourneyStep, JourneyLoginSuccess, JourneyLoginFailure, or GenericError if the step type cannot be determined + */ function createJourneyObject( step: Step, -): JourneyStep | JourneyLoginSuccess | JourneyLoginFailure | undefined { +): JourneyStep | JourneyLoginSuccess | JourneyLoginFailure | GenericError { let type; if (step.authId) { type = StepType.Step; @@ -37,7 +45,11 @@ function createJourneyObject( case StepType.Step: return createJourneyStep(step); default: - return undefined; + return { + error: 'unknown_step_type', + message: 'Unable to determine step type from server response', + type: 'unknown_error', + }; } }