Refactor getDiscussion (#10361)

* refactor(getDiscussion): more accurate parsing

and safer error handling

See guardian/discussion-api for type definitions

* feat(result): success and failure wrapper

* feat(JSON): wrap in Result type

---------

Co-authored-by: Jamie B <[email protected]>

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;

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;

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;

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;

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;

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);

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<DiscussionResponse>(
+	const { data: discussionData } = useApi<GetDiscussionSuccess>(
 		joinUrl(discussionApiUrl, 'discussion', shortUrlId),
 		{
 			// The default for dedupingInterval is 2 seconds but we want to wait longer here because the cache time

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<DiscussionResponse | undefined> => {
+): Promise<Result<GetDiscussionError, GetDiscussionSuccess>> => {
 	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<string> => {

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<typeof fetch>
+): Promise<Result<'ApiError' | 'NetworkError', unknown>> => {
+	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' };
+	}
+};

dotcom-rendering/src/lib/result.ts

@@ -0,0 +1,11 @@
+type Result<Err, Value> =
+	| {
+			kind: 'ok';
+			value: Value;
+	  }
+	| {
+			kind: 'error';
+			error: Err;
+	  };
+
+export type { Result };

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;