Skip to content

Commit

Permalink
Fix error propagation (#82)
Browse files Browse the repository at this point in the history
  • Loading branch information
marcospassos authored Nov 14, 2024
1 parent 7e225ac commit 6b82abd
Show file tree
Hide file tree
Showing 6 changed files with 82 additions and 32 deletions.
34 changes: 30 additions & 4 deletions src/headers.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,24 @@ describe('getHeaders', () => {
});

it('should require the request context if next/headers is not available', () => {
const error = new Error('next/headers requires app router');

mockHeaders.mockImplementation(() => {
throw new Error('next/headers requires app router');
throw error;
});

expect(() => getHeaders()).toThrow('No route context found.');
let actualError: unknown;

try {
getHeaders();
} catch (caughtError) {
actualError = caughtError;
}

// Ensure it rethrows the exact same error.
// It is important because Next uses the error type to detect dynamic routes based
// on usage of headers or cookies
expect(actualError).toBe(error);
});

type RequestContextScenario = {
Expand Down Expand Up @@ -138,11 +151,24 @@ describe('getCookies', () => {
});

it('should require the request context if next/headers is not available', () => {
const error = new Error('next/headers requires app router');

mockCookies.mockImplementation(() => {
throw new Error('next/headers requires app router');
throw error;
});

expect(() => getCookies()).toThrow('No route context found.');
let actualError: unknown;

try {
getCookies();
} catch (caughtError) {
actualError = caughtError;
}

// Ensure it rethrows the exact same error.
// It is important because Next uses the error type to detect dynamic routes based
// on usage of headers or cookies
expect(actualError).toBe(error);
});

type RequestContextScenario = {
Expand Down
8 changes: 4 additions & 4 deletions src/headers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@ export function getHeaders(route?: RouteContext): HeaderReader {
const {headers} = importNextHeaders();

return headers();
} catch {
} catch (error) {
if (route === undefined) {
throw new Error('No route context found.');
throw error;
}
}

Expand Down Expand Up @@ -70,9 +70,9 @@ export function getCookies(route?: RouteContext): CookieAccessor {
const {cookies} = importNextHeaders();

return cookies();
} catch {
} catch (error) {
if (route === undefined) {
throw new Error('No route context found.');
throw error;
}
}

Expand Down
15 changes: 13 additions & 2 deletions src/server/evaluate.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {ApiKey, ApiKey as MockApiKey} from '@croct/sdk/apiKey';
import {FilteredLogger} from '@croct/sdk/logging/filteredLogger';
import {headers} from 'next/headers';
import {NextRequest, NextResponse} from 'next/server';
import {DynamicServerError} from 'next/dist/client/components/hooks-server-context';
import {cql, evaluate, EvaluationOptions} from './evaluate';
import {resolveRequestContext, RequestContext} from '@/config/context';
import {getDefaultFetchTimeout} from '@/config/timeout';
Expand Down Expand Up @@ -186,15 +187,25 @@ describe('evaluation', () => {
expect(resolveRequestContext).toHaveBeenCalledWith(route);
});

it('should rethrow dynamic server errors', async () => {
const error = new DynamicServerError('cause');

jest.mocked(resolveRequestContext).mockImplementation(() => {
throw error;
});

await expect(evaluate('true')).rejects.toBe(error);
});

it('should report an error if the route context is missing', async () => {
jest.mocked(resolveRequestContext).mockImplementation(() => {
throw new Error('next/headers requires app router');
});

await expect(evaluate('true')).rejects.toThrow(
'Error resolving request context: next/headers requires app router. '
+ 'This error usually occurs when no `route` option is specified when evaluate() '
+ 'is called outside of app routes. '
+ 'This error typically occurs when evaluate() is called outside of app routes '
+ 'without specifying the `route` option. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
);
});
Expand Down
21 changes: 11 additions & 10 deletions src/server/evaluate.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import type {JsonValue} from '@croct/plug-react';
import {FilteredLogger} from '@croct/sdk/logging/filteredLogger';
import {ConsoleLogger} from '@croct/sdk/logging/consoleLogger';
import {formatCause} from '@croct/sdk/error';
import {isDynamicServerError} from 'next/dist/client/components/hooks-server-context';
import {getApiKey} from '@/config/security';
import {RequestContext, resolveRequestContext} from '@/config/context';
import {getDefaultFetchTimeout} from '@/config/timeout';
Expand All @@ -21,18 +22,18 @@ export function evaluate<T extends JsonValue>(query: string, options: Evaluation
try {
context = resolveRequestContext(route);
} catch (error) {
if (route === undefined) {
return Promise.reject(
new Error(
`Error resolving request context: ${formatCause(error)}. `
+ 'This error usually occurs when no `route` option is specified when evaluate() '
+ 'is called outside of app routes. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
),
);
if (isDynamicServerError(error) || route !== undefined) {
return Promise.reject(error);
}

return Promise.reject(error);
return Promise.reject(
new Error(
`Error resolving request context: ${formatCause(error)}. `
+ 'This error typically occurs when evaluate() is called outside of app routes '
+ 'without specifying the `route` option. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
),
);
}

const timeout = getDefaultFetchTimeout();
Expand Down
15 changes: 13 additions & 2 deletions src/server/fetchContent.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {FetchResponse} from '@croct/plug/plug';
import {ApiKey, ApiKey as MockApiKey} from '@croct/sdk/apiKey';
import {FilteredLogger} from '@croct/sdk/logging/filteredLogger';
import type {NextRequest, NextResponse} from 'next/server';
import {DynamicServerError} from 'next/dist/client/components/hooks-server-context';
import {fetchContent, FetchOptions} from './fetchContent';
import {RequestContext, resolvePreferredLocale, resolveRequestContext} from '@/config/context';
import {getDefaultFetchTimeout} from '@/config/timeout';
Expand Down Expand Up @@ -289,15 +290,25 @@ describe('fetchContent', () => {
expect(resolvePreferredLocale).toHaveBeenCalledWith(route);
});

it('should rethrow dynamic server errors', async () => {
const error = new DynamicServerError('cause');

jest.mocked(resolveRequestContext).mockImplementation(() => {
throw error;
});

await expect(fetchContent('slot-id')).rejects.toBe(error);
});

it('should report an error if the route context is missing', async () => {
jest.mocked(resolveRequestContext).mockImplementation(() => {
throw new Error('next/headers requires app router');
});

await expect(fetchContent('slot-id')).rejects.toThrow(
'Error resolving request context: next/headers requires app router. '
+ 'This error usually occurs when no `route` option is specified when fetchContent() '
+ 'is called outside of app routes. '
+ 'This error typically occurs when fetchContent() is called outside of app routes without '
+ 'specifying the `route` option. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
);
});
Expand Down
21 changes: 11 additions & 10 deletions src/server/fetchContent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {SlotContent, VersionedSlotId, JsonObject} from '@croct/plug-react';
import {FilteredLogger} from '@croct/sdk/logging/filteredLogger';
import {ConsoleLogger} from '@croct/sdk/logging/consoleLogger';
import {formatCause} from '@croct/sdk/error';
import {isDynamicServerError} from 'next/dist/client/components/hooks-server-context';
import {getApiKey} from '@/config/security';
import {RequestContext, resolvePreferredLocale, resolveRequestContext} from '@/config/context';
import {getDefaultFetchTimeout} from '@/config/timeout';
Expand Down Expand Up @@ -63,18 +64,18 @@ export function fetchContent<I extends VersionedSlotId, C extends JsonObject>(
try {
context = resolveRequestContext(route);
} catch (error) {
if (route === undefined) {
return Promise.reject(
new Error(
`Error resolving request context: ${formatCause(error)}. `
+ 'This error usually occurs when no `route` option is specified when fetchContent() '
+ 'is called outside of app routes. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
),
);
if (isDynamicServerError(error) || route !== undefined) {
return Promise.reject(error);
}

return Promise.reject(error);
return Promise.reject(
new Error(
`Error resolving request context: ${formatCause(error)}. `
+ 'This error typically occurs when fetchContent() is called outside of app routes '
+ 'without specifying the `route` option. '
+ 'For help, see: https://croct.help/sdk/nextjs/missing-route-context',
),
);
}

return loadContent<I, C>(slotId, {
Expand Down

0 comments on commit 6b82abd

Please sign in to comment.