From a959fb72c00b06d477cf852177d11a7075f92f5d Mon Sep 17 00:00:00 2001 From: Max Duval Date: Fri, 26 Jan 2024 13:14:21 +0000 Subject: [PATCH] refactor(getDiscussion): more accurate parsing and safer error handling See guardian/discussion-api for type definitions Co-authored-by: Jamie B <53781962+JamieB-gu@users.noreply.github.com> --- .../src/components/Discussion.tsx | 20 ++++-- .../components/DiscussionMeta.importable.tsx | 23 ++++++- dotcom-rendering/src/lib/discussionApi.tsx | 56 ++++++++++------ dotcom-rendering/src/lib/result.ts | 11 ++++ dotcom-rendering/src/types/discussion.ts | 64 ++++++++++--------- 5 files changed, 119 insertions(+), 55 deletions(-) create mode 100644 dotcom-rendering/src/lib/result.ts 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..d0ac7d1cbb6 100644 --- a/dotcom-rendering/src/components/DiscussionMeta.importable.tsx +++ b/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, diff --git a/dotcom-rendering/src/lib/discussionApi.tsx b/dotcom-rendering/src/lib/discussionApi.tsx index efe8b749702..5287ba572fa 100644 --- a/dotcom-rendering/src/lib/discussionApi.tsx +++ b/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 => { +): Promise> => { 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 => { 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..8cf1966094c 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 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;