Skip to content

Commit 0a40c11

Browse files
ryanbas21ryanbas21
committed
fix!: update-error-handling-across-packages
1 parent 4933f55 commit 0a40c11

15 files changed

Lines changed: 116 additions & 95 deletions

File tree

‎.changeset/chilly-sheep-follow.md‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
'@forgerock/journey-client': major
3+
---
4+
5+
**BREAKING CHANGE**: Journey client methods now return `GenericError` instead of `undefined` for error cases.
6+
7+
## What Changed
8+
9+
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.
10+
11+
### Return Type Changes
12+
13+
| Method | Before | After |
14+
| ----------- | ------------------------------------------------------------------------ | --------------------------------------------------------------------------- |
15+
| `start` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` |
16+
| `next` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` |
17+
| `resume` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| undefined` | `JourneyStep \| JourneyLoginSuccess \| JourneyLoginFailure \| GenericError` |
18+
| `terminate` | `void` | `void \| GenericError` |
19+
20+
## Migration Guide
21+
22+
Before:
23+
24+
```ts
25+
const step = await client.start();
26+
if (step) {
27+
// Use step
28+
}
29+
30+
After: const result = await client.start();
31+
if ('error' in result) {
32+
// Handle error
33+
console.error(result.message);
34+
} else {
35+
// Use step
36+
}
37+
```

‎e2e/davinci-app/tsconfig.json‎

Lines changed: 0 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -14,15 +14,6 @@
1414
"skipLibCheck": true
1515
},
1616
"references": [
17-
{
18-
"path": "../../packages/sdk-effects/logger"
19-
},
20-
{
21-
"path": "../../packages/protect"
22-
},
23-
{
24-
"path": "../../packages/davinci-client"
25-
},
2617
{
2718
"path": "./tsconfig.app.json"
2819
},

‎e2e/device-client-app/tsconfig.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
"files": [],
44
"include": [],
55
"references": [
6-
{
7-
"path": "../../packages/device-client"
8-
},
96
{
107
"path": "./tsconfig.app.json"
118
}

‎e2e/journey-app/tsconfig.json‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -14,18 +14,6 @@
1414
"skipLibCheck": true
1515
},
1616
"references": [
17-
{
18-
"path": "../../packages/sdk-effects/logger"
19-
},
20-
{
21-
"path": "../../packages/oidc-client"
22-
},
23-
{
24-
"path": "../../packages/protect"
25-
},
26-
{
27-
"path": "../../packages/journey-client"
28-
},
2917
{
3018
"path": "./tsconfig.app.json"
3119
},

‎e2e/oidc-app/tsconfig.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -3,9 +3,6 @@
33
"files": [],
44
"include": [],
55
"references": [
6-
{
7-
"path": "../../packages/oidc-client"
8-
},
96
{
107
"path": "./tsconfig.app.json"
118
}

‎e2e/protect-app/tsconfig.json‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -16,9 +16,6 @@
1616
"skipLibCheck": true
1717
},
1818
"references": [
19-
{
20-
"path": "../../packages/protect"
21-
},
2219
{
2320
"path": "./tsconfig.app.json"
2421
},

‎packages/journey-client/src/lib/client.store.test.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -8,12 +8,19 @@
88
import { callbackType } from '@forgerock/sdk-types';
99
import { afterEach, describe, expect, test, vi } from 'vitest';
1010

11-
import type { Step } from '@forgerock/sdk-types';
11+
import type { GenericError, Step } from '@forgerock/sdk-types';
1212

1313
import { journey } from './client.store.js';
1414
import { createJourneyStep } from './step.utils.js';
1515
import { JourneyClientConfig } from './config.types.js';
1616

17+
/**
18+
* Type guard to check if a result is a GenericError
19+
*/
20+
function isGenericError(result: unknown): result is GenericError {
21+
return typeof result === 'object' && result !== null && 'error' in result && 'type' in result;
22+
}
23+
1724
// Create a singleton mock instance for storage
1825
const mockStorageInstance = {
1926
get: vi.fn(),
@@ -65,13 +72,16 @@ describe('journey-client', () => {
6572
const client = await journey({ config: mockConfig });
6673
const step = await client.start();
6774
expect(step).toBeDefined();
75+
expect(isGenericError(step)).toBe(false);
6876

6977
expect(mockFetch).toHaveBeenCalledTimes(1);
7078
const request = mockFetch.mock.calls[0][0] as Request;
7179
// TODO: This should be /journeys?_action=start, but the current implementation calls /authenticate
7280
expect(request.url).toBe('https://test.com/json/realms/root/authenticate');
7381
expect(step).toHaveProperty('type', 'Step');
74-
expect(step && step.payload).toEqual(mockStepResponse);
82+
if (!isGenericError(step)) {
83+
expect(step.payload).toEqual(mockStepResponse);
84+
}
7585
});
7686

7787
test('next() should send the current step and return the next step', async () => {
@@ -101,6 +111,7 @@ describe('journey-client', () => {
101111
const client = await journey({ config: mockConfig });
102112
const nextStep = await client.next(initialStep, {});
103113
expect(nextStep).toBeDefined();
114+
expect(isGenericError(nextStep)).toBe(false);
104115

105116
expect(mockFetch).toHaveBeenCalledTimes(1);
106117
const request = mockFetch.mock.calls[0][0] as Request;
@@ -109,7 +120,9 @@ describe('journey-client', () => {
109120
expect(request.method).toBe('POST');
110121
expect(await request.json()).toEqual(initialStep.payload);
111122
expect(nextStep).toHaveProperty('type', 'Step');
112-
expect(nextStep && nextStep.payload).toEqual(nextStepPayload);
123+
if (!isGenericError(nextStep)) {
124+
expect(nextStep.payload).toEqual(nextStepPayload);
125+
}
113126
});
114127

115128
test('redirect() should store the step and call location.assign', async () => {
@@ -169,7 +182,9 @@ describe('journey-client', () => {
169182
expect(request.method).toBe('POST');
170183
expect(await request.json()).toEqual(previousStepPayload);
171184
expect(step).toHaveProperty('type', 'Step');
172-
expect(step && step.payload).toEqual(nextStepPayload);
185+
if (!isGenericError(step)) {
186+
expect(step.payload).toEqual(nextStepPayload);
187+
}
173188
});
174189

175190
test('should correctly resume with a plain Step object from storage', async () => {
@@ -198,7 +213,9 @@ describe('journey-client', () => {
198213
expect(request.method).toBe('POST');
199214
expect(await request.json()).toEqual(plainStepPayload); // Expect the plain payload to be sent
200215
expect(step).toHaveProperty('type', 'Step'); // The returned step should still be an JourneyStep instance
201-
expect(step && step.payload).toEqual(nextStepPayload);
216+
if (!isGenericError(step)) {
217+
expect(step.payload).toEqual(nextStepPayload);
218+
}
202219
});
203220

204221
test('should throw an error if a previous step is required but not found', async () => {
@@ -234,7 +251,9 @@ describe('journey-client', () => {
234251
const url = new URL(request.url);
235252
expect(url.origin + url.pathname).toBe('https://test.com/json/realms/root/authenticate');
236253
expect(step).toHaveProperty('type', 'Step');
237-
expect(step && step.payload).toEqual(mockStepResponse);
254+
if (!isGenericError(step)) {
255+
expect(step.payload).toEqual(mockStepResponse);
256+
}
238257
});
239258
});
240259

‎packages/journey-client/src/lib/client.store.ts‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -70,19 +70,36 @@ export async function journey({
7070
const self = {
7171
start: async (options?: StartParam) => {
7272
const { data } = await store.dispatch(journeyApi.endpoints.start.initiate(options));
73-
return data ? createJourneyObject(data) : undefined;
73+
if (!data) {
74+
const error: GenericError = {
75+
error: 'no_response_data',
76+
message: 'No data received from server when starting journey',
77+
type: 'unknown_error',
78+
};
79+
return error;
80+
}
81+
return createJourneyObject(data);
7482
},
7583

7684
/**
7785
* Submits the current Step payload to the authentication API and retrieves the next JourneyStep in the journey.
7886
* The `step` to be submitted is provided within the `options` object.
7987
*
80-
* @param options An object containing the current Step payload and optional JourneyClientConfig.
81-
* @returns A Promise that resolves to the next JourneyStep in the journey, or undefined if the journey ends.
88+
* @param step The current JourneyStep containing user input to submit.
89+
* @param options Optional configuration for the request.
90+
* @returns A Promise that resolves to a JourneyStep, JourneyLoginSuccess, JourneyLoginFailure, or GenericError.
8291
*/
8392
next: async (step: JourneyStep, options?: NextOptions) => {
8493
const { data } = await store.dispatch(journeyApi.endpoints.next.initiate({ step, options }));
85-
return data ? createJourneyObject(data) : undefined;
94+
if (!data) {
95+
const error: GenericError = {
96+
error: 'no_response_data',
97+
message: 'No data received from server when submitting step',
98+
type: 'unknown_error',
99+
};
100+
return error;
101+
}
102+
return createJourneyObject(data);
86103
},
87104

88105
// TODO: Remove the actual redirect from this method and just return the URL to the caller
@@ -108,7 +125,7 @@ export async function journey({
108125
resume: async (
109126
url: string,
110127
options?: ResumeOptions,
111-
): Promise<ReturnType<typeof createJourneyObject> | undefined> => {
128+
): Promise<ReturnType<typeof createJourneyObject>> => {
112129
const parsedUrl = new URL(url);
113130
const code = parsedUrl.searchParams.get('code');
114131
const state = parsedUrl.searchParams.get('state');
@@ -183,11 +200,22 @@ export async function journey({
183200
* This will invalidate the current session and clean up any server-side state.
184201
*
185202
* @param options Optional StepOptions containing query parameters.
186-
* @returns A Promise that resolves when the session is successfully ended.
203+
* @returns A Promise that resolves to void on success, or GenericError on failure.
187204
*/
188-
terminate: async (options?: { query?: Record<string, string> }) => {
189-
const { data } = await store.dispatch(journeyApi.endpoints.terminate.initiate(options));
190-
return data;
205+
terminate: async (options?: {
206+
query?: Record<string, string>;
207+
}): Promise<void | GenericError> => {
208+
const { error } = await store.dispatch(journeyApi.endpoints.terminate.initiate(options));
209+
if (error) {
210+
return {
211+
error: 'terminate_failed',
212+
message:
213+
'status' in error
214+
? `Failed to terminate session: ${error.status}`
215+
: 'Failed to terminate session',
216+
type: 'unknown_error',
217+
};
218+
}
191219
},
192220
};
193221
return self;

‎packages/journey-client/src/lib/device/device-profile.test.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77
* This software may be modified and distributed under the terms
88
* of the MIT license. See the LICENSE file for details.
99
*/
10-
import { vi, expect, describe, it, afterEach, beforeEach, SpyInstance } from 'vitest';
10+
import { vi, expect, describe, it, afterEach, beforeEach, MockInstance } from 'vitest';
1111

1212
import { Device } from './device-profile.js';
1313

@@ -86,7 +86,7 @@ describe('Test DeviceProfile', () => {
8686
});
8787

8888
describe('logLevel tests', () => {
89-
let warnSpy: SpyInstance;
89+
let warnSpy: MockInstance;
9090
const originalNavigator = global.navigator;
9191

9292
beforeEach(() => {

‎packages/journey-client/src/lib/journey.utils.ts‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@
77

88
import { StepType } from '@forgerock/sdk-types';
99

10-
import type { Step } from '@forgerock/sdk-types';
10+
import type { GenericError, Step } from '@forgerock/sdk-types';
1111

1212
import { createJourneyLoginSuccess } from './login-success.utils.js';
1313
import { createJourneyLoginFailure } from './login-failure.utils.js';
@@ -17,9 +17,17 @@ import type { JourneyStep } from './step.utils.js';
1717
import type { JourneyLoginFailure } from './login-failure.utils.js';
1818
import type { JourneyLoginSuccess } from './login-success.utils.js';
1919

20+
/**
21+
* Creates a journey object from a raw Step response.
22+
* Determines the step type based on the presence of authId or successUrl properties
23+
* and returns the appropriate journey object type.
24+
*
25+
* @param step - The raw Step response from the authentication API
26+
* @returns A JourneyStep, JourneyLoginSuccess, JourneyLoginFailure, or GenericError if the step type cannot be determined
27+
*/
2028
function createJourneyObject(
2129
step: Step,
22-
): JourneyStep | JourneyLoginSuccess | JourneyLoginFailure | undefined {
30+
): JourneyStep | JourneyLoginSuccess | JourneyLoginFailure | GenericError {
2331
let type;
2432
if (step.authId) {
2533
type = StepType.Step;
@@ -37,7 +45,11 @@ function createJourneyObject(
3745
case StepType.Step:
3846
return createJourneyStep(step);
3947
default:
40-
return undefined;
48+
return {
49+
error: 'unknown_step_type',
50+
message: 'Unable to determine step type from server response',
51+
type: 'unknown_error',
52+
};
4153
}
4254
}
4355

0 commit comments

Comments
 (0)