Skip to content
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

Fix error propagation #82

Merged
merged 1 commit into from
Nov 14, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 {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 @@
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 Expand Up @@ -235,12 +246,12 @@
logger?.warn('warning');
logger?.error('error');

expect(console.info).not.toHaveBeenCalled();

Check warning on line 249 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.debug).not.toHaveBeenCalled();

Check warning on line 250 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.warn).toHaveBeenCalledWith('warning');

Check warning on line 251 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.error).toHaveBeenCalledWith('error');

Check warning on line 252 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement

expect(console.log).not.toHaveBeenCalled();

Check warning on line 254 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
});

it('should log all messages if the debug mode is enabled', async () => {
Expand All @@ -266,12 +277,12 @@
logger?.warn('warning');
logger?.error('error');

expect(console.info).toHaveBeenCalledWith('info');

Check warning on line 280 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.debug).toHaveBeenCalledWith('debug');

Check warning on line 281 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.warn).toHaveBeenCalledWith('warning');

Check warning on line 282 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
expect(console.error).toHaveBeenCalledWith('error');

Check warning on line 283 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement

expect(console.log).not.toHaveBeenCalled();

Check warning on line 285 in src/server/evaluate.test.ts

View workflow job for this annotation

GitHub Actions / lint

Unexpected console statement
});

it('should use the base endpoint URL from the environment', async () => {
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