refactor(getDiscussion): more accurate parsing

and safer error handling

See guardian/discussion-api for type definitions

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

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,6 @@ 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 { SignedInAs } from './SignedInAs';
 
 type Props = {
@@ -12,6 +11,28 @@ type Props = {
 	enableDiscussionSwitch: boolean;
 };
 
+/** @deprecated when we unify the state we will no longer need this extra network call */
+type DiscussionResponse = {
+	// status: string;
+	// errorCode?: string;
+	// 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[];
+	};
+};
+
 export const DiscussionMeta = ({
 	discussionApiUrl,
 	shortUrlId,

dotcom-rendering/src/lib/discussionApi.tsx

@@ -1,17 +1,19 @@
 import { joinUrl } from '@guardian/libs';
+import { safeParse } from 'valibot';
 import type {
 	AdditionalHeadersType,
 	CommentResponse,
 	CommentType,
 	DiscussionOptions,
-	DiscussionResponse,
+	DiscussionSuccess,
 	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 type { Result } from './result';
 
 const options = {
 	// Defaults
@@ -56,6 +58,8 @@ const objAsParams = (obj: any): string => {
 	return '?' + params;
 };
 
+type GetDiscussionError = 'Parsing error' | 'ApiError' | 'NetworkError';
+
 //todo: figure out the different return types and consider error handling
 export const getDiscussion = async (
 	shortUrl: string,
@@ -65,7 +69,7 @@ export const getDiscussion = async (
 		threads: ThreadsType;
 		page: number;
 	},
-): Promise<DiscussionResponse | undefined> => {
+): Promise<Result<GetDiscussionError, DiscussionSuccess>> => {
 	const apiOpts: DiscussionOptions = {
 		...defaultParams,
 		...{
@@ -85,24 +89,40 @@ export const getDiscussion = async (
 
 	const url = joinUrl(options.baseUrl, 'discussion', shortUrl) + params;
 
-	const resp = await fetch(url, {
+	const json = (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;
+	})
+		.then((r) => r.json())
+		.catch(() => undefined)) as unknown;
+
+	if (!json) return { kind: 'error', error: 'NetworkError' };
+
+	const result = safeParse(discussionApiResponseSchema, json);
+	if (!result.success) {
+		return { kind: 'error', error: 'Parsing error' };
+	}
+	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/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 DiscussionSuccess = {
+	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;