diff --git a/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts b/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts index 2ae9766c48c..854d5c8c7ae 100644 --- a/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts +++ b/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts @@ -1,4 +1,4 @@ -import type { DiscussionResponse } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussionNoTopComments = { status: 'ok', @@ -18,4 +18,4 @@ export const discussionNoTopComments = { webUrl: 'https://www.theguardian.com/music/2014/jul/25/stevie-nicks-ro-release-double-album-of-songs-from-her-past', comments: [], }, -} satisfies DiscussionResponse; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussion.ts b/dotcom-rendering/fixtures/manual/discussion.ts index fe1cd099b69..7b2ecc496c8 100644 --- a/dotcom-rendering/fixtures/manual/discussion.ts +++ b/dotcom-rendering/fixtures/manual/discussion.ts @@ -1,4 +1,4 @@ -import type { DiscussionResponse } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussion = { status: 'ok', @@ -1183,4 +1183,4 @@ export const discussion = { }, ], }, -} satisfies DiscussionResponse; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts index 77b0ea5d18f..87d420fd255 100644 --- a/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts +++ b/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts @@ -1,4 +1,4 @@ -import type { DiscussionResponse } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussionWithNoComments = { status: 'ok', @@ -18,4 +18,4 @@ export const discussionWithNoComments = { title: 'Mystery bird: black-and-red broadbill, Cymbirhynchus macrorhynchos story', comments: [], }, -} satisfies DiscussionResponse; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts index da50b586e6d..d56c428ae56 100644 --- a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts +++ b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts @@ -1,4 +1,4 @@ -import type { DiscussionResponse } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussionWithTwoComments = { status: 'ok', @@ -63,4 +63,4 @@ export const discussionWithTwoComments = { }, ], }, -} satisfies DiscussionResponse; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/short-discussion.ts b/dotcom-rendering/fixtures/manual/short-discussion.ts index e75b682d9fc..5a146683fa0 100644 --- a/dotcom-rendering/fixtures/manual/short-discussion.ts +++ b/dotcom-rendering/fixtures/manual/short-discussion.ts @@ -1,4 +1,4 @@ -import type { DiscussionResponse } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const shortDiscussion = { status: 'ok', @@ -59,4 +59,4 @@ export const shortDiscussion = { }, ], }, -} satisfies DiscussionResponse; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/src/components/Discussion.tsx b/dotcom-rendering/src/components/Discussion.tsx index c941c748d3f..e134da3db4a 100644 --- a/dotcom-rendering/src/components/Discussion.tsx +++ b/dotcom-rendering/src/components/Discussion.tsx @@ -112,15 +112,21 @@ export const Discussion = ({ useEffect(() => { setLoading(true); void getDiscussion(shortUrlId, { ...filters, page: commentPage }) - .then((json) => { - setLoading(false); - if (json && json.status !== 'error') { - setComments(json.discussion.comments); - setIsClosedForComments(json.discussion.isClosedForComments); + .then((result) => { + if (result.kind === 'error') { + console.error(`getDiscussion - error: ${result.error}`); + return; } - if (json?.pages != null) setTotalPages(json.pages); + + setLoading(false); + const { pages, discussion } = result.value; + setComments(discussion.comments); + setIsClosedForComments(discussion.isClosedForComments); + setTotalPages(pages); }) - .catch((e) => console.error(`getDiscussion - error: ${String(e)}`)); + .catch(() => { + // do nothing + }); }, [filters, commentPage, shortUrlId]); const validFilters = remapFilters(filters, hashCommentId); diff --git a/dotcom-rendering/src/components/DiscussionMeta.importable.tsx b/dotcom-rendering/src/components/DiscussionMeta.importable.tsx index d48c547cb52..1442e35d747 100644 --- a/dotcom-rendering/src/components/DiscussionMeta.importable.tsx +++ b/dotcom-rendering/src/components/DiscussionMeta.importable.tsx @@ -3,7 +3,7 @@ import { getOptionsHeadersWithOkta } from '../lib/identity'; import { useApi } from '../lib/useApi'; import { useAuthStatus } from '../lib/useAuthStatus'; import { useCommentCount } from '../lib/useCommentCount'; -import type { DiscussionResponse } from '../types/discussion'; +import type { GetDiscussionSuccess } from '../types/discussion'; import { SignedInAs } from './SignedInAs'; type Props = { @@ -20,7 +20,7 @@ export const DiscussionMeta = ({ const authStatus = useAuthStatus(); const commentCount = useCommentCount(discussionApiUrl, shortUrlId); - const { data: discussionData } = useApi( + const { data: discussionData } = useApi( joinUrl(discussionApiUrl, 'discussion', shortUrlId), { // The default for dedupingInterval is 2 seconds but we want to wait longer here because the cache time diff --git a/dotcom-rendering/src/lib/discussionApi.tsx b/dotcom-rendering/src/lib/discussionApi.tsx index efe8b749702..7d48706126f 100644 --- a/dotcom-rendering/src/lib/discussionApi.tsx +++ b/dotcom-rendering/src/lib/discussionApi.tsx @@ -1,17 +1,20 @@ import { joinUrl } from '@guardian/libs'; +import { safeParse } from 'valibot'; import type { AdditionalHeadersType, CommentResponse, CommentType, DiscussionOptions, - DiscussionResponse, + GetDiscussionSuccess, OrderByType, ThreadsType, UserNameResponse, } from '../types/discussion'; -import { parseDiscussionResponse } from '../types/discussion'; +import { discussionApiResponseSchema } from '../types/discussion'; import type { SignedInWithCookies, SignedInWithOkta } from './identity'; import { getOptionsHeadersWithOkta } from './identity'; +import { fetchJSON } from './json'; +import type { Result } from './result'; const options = { // Defaults @@ -56,6 +59,8 @@ const objAsParams = (obj: any): string => { return '?' + params; }; +type GetDiscussionError = 'ParsingError' | 'ApiError' | 'NetworkError'; + //todo: figure out the different return types and consider error handling export const getDiscussion = async ( shortUrl: string, @@ -65,7 +70,7 @@ export const getDiscussion = async ( threads: ThreadsType; page: number; }, -): Promise => { +): Promise> => { const apiOpts: DiscussionOptions = { ...defaultParams, ...{ @@ -85,24 +90,34 @@ export const getDiscussion = async ( const url = joinUrl(options.baseUrl, 'discussion', shortUrl) + params; - const resp = await fetch(url, { - headers: { - ...options.headers, - }, - }); - - const discussionResponse = parseDiscussionResponse(await resp.json()); - - return discussionResponse?.errorCode === - 'DISCUSSION_ONLY_AVAILABLE_IN_LINEAR_FORMAT' - ? // We need force a refetch with unthreaded set, as we don't know - // that this discussion is only available in linear format until - // we get the response to tell us - getDiscussion(shortUrl, { - ...opts, - ...{ threads: 'unthreaded' }, - }) - : discussionResponse; + const jsonResult = await fetchJSON(url, { headers: options.headers }); + + if (jsonResult.kind === 'error') return jsonResult; + + const result = safeParse(discussionApiResponseSchema, jsonResult.value); + if (!result.success) { + return { kind: 'error', error: 'ParsingError' }; + } + if ( + result.output.status === 'error' && + // We need force a refetch with unthreaded set, as we don't know + // that this discussion is only available in linear format until + // we get the response to tell us + result.output.errorCode === 'DISCUSSION_ONLY_AVAILABLE_IN_LINEAR_FORMAT' + ) { + return getDiscussion(shortUrl, { + ...opts, + ...{ threads: 'unthreaded' }, + }); + } + if (result.output.status === 'error') { + return { + kind: 'error', + error: 'ApiError', + }; + } + + return { kind: 'ok', value: result.output }; }; export const preview = async (body: string): Promise => { diff --git a/dotcom-rendering/src/lib/json.ts b/dotcom-rendering/src/lib/json.ts new file mode 100644 index 00000000000..63db15a51ea --- /dev/null +++ b/dotcom-rendering/src/lib/json.ts @@ -0,0 +1,21 @@ +import type { Result } from './result'; + +/** + * Safely fetch JSON from a URL. + * + * If successful, the value is typed as `unknown`. + */ +export const fetchJSON = async ( + ...args: Parameters +): Promise> => { + try { + const response = await fetch(...args); + return { kind: 'ok', value: await response.json() }; + } catch (error) { + if (error instanceof SyntaxError) { + return { kind: 'error', error: 'ApiError' }; + } + + return { kind: 'error', error: 'NetworkError' }; + } +}; diff --git a/dotcom-rendering/src/lib/result.ts b/dotcom-rendering/src/lib/result.ts new file mode 100644 index 00000000000..66141177ffb --- /dev/null +++ b/dotcom-rendering/src/lib/result.ts @@ -0,0 +1,11 @@ +type Result = + | { + kind: 'ok'; + value: Value; + } + | { + kind: 'error'; + error: Err; + }; + +export type { Result }; diff --git a/dotcom-rendering/src/types/discussion.ts b/dotcom-rendering/src/types/discussion.ts index 23c49eb333c..e13cbe017c8 100644 --- a/dotcom-rendering/src/types/discussion.ts +++ b/dotcom-rendering/src/types/discussion.ts @@ -2,12 +2,14 @@ import type { BaseSchema } from 'valibot'; import { array, boolean, + literal, number, object, optional, recursive, - safeParse, string, + unknown, + variant, } from 'valibot'; import type { Guard } from '../lib/guard'; import { guard } from '../lib/guard'; @@ -226,30 +228,38 @@ export type SignedInUser = { authStatus: SignedInWithCookies | SignedInWithOkta; }; -export interface DiscussionResponse { - status: string; - errorCode?: string; +const discussionApiErrorSchema = object({ + status: literal('error'), + statusCode: number(), + message: string(), + errorCode: optional(string()), +}); + +export type GetDiscussionSuccess = { + status: 'ok'; currentPage: number; pages: number; pageSize: number; orderBy: string; - discussion: { - key: string; - webUrl: string; - apiUrl: string; - commentCount: number; - topLevelCommentCount: number; - isClosedForComments: boolean; - isClosedForRecommendation: boolean; - isThreaded: boolean; - title: string; - comments: CommentType[]; - }; -} + discussion: DiscussionData; + switches?: unknown; +}; -const discussionResponse = object({ - status: string(), - errorCode: optional(string()), +type DiscussionData = { + key: string; + webUrl: string; + apiUrl: string; + commentCount: number; + topLevelCommentCount: number; + isClosedForComments: boolean; + isClosedForRecommendation: boolean; + isThreaded: boolean; + title: string; + comments: CommentType[]; +}; + +const discussionApiSuccessSchema = object({ + status: literal('ok'), currentPage: number(), pages: number(), pageSize: number(), @@ -266,17 +276,13 @@ const discussionResponse = object({ title: string(), comments: array(comment), }), + switches: unknown(), }); -export const parseDiscussionResponse = ( - data: unknown, -): DiscussionResponse | undefined => { - const result = safeParse(discussionResponse, data); - if (!result.success) { - console.error('DiscussionResponse', result.issues); - } - return result.success ? result.output : undefined; -}; +export const discussionApiResponseSchema = variant('status', [ + discussionApiErrorSchema, + discussionApiSuccessSchema, +]); export interface DiscussionOptions { orderBy: string;