From fedd1fea7ace8dfd59fb0af8c9d9d295191e0d0f Mon Sep 17 00:00:00 2001 From: Max Duval Date: Fri, 26 Jan 2024 13:14:21 +0000 Subject: [PATCH 1/4] 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> --- .../manual/discussion-no-top-comments.ts | 4 +- .../fixtures/manual/discussion.ts | 4 +- .../manual/discussionWithNoComments.ts | 4 +- .../manual/discussionWithTwoComments.ts | 4 +- .../fixtures/manual/short-discussion.ts | 4 +- .../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 ++++++++++--------- 10 files changed, 129 insertions(+), 65 deletions(-) create mode 100644 dotcom-rendering/src/lib/result.ts diff --git a/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts b/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts index 2ae9766c48c..4e3812d85ce 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 { DiscussionSuccess } 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 DiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussion.ts b/dotcom-rendering/fixtures/manual/discussion.ts index fe1cd099b69..24494632b3c 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 { DiscussionSuccess } from '../../src/types/discussion'; export const discussion = { status: 'ok', @@ -1183,4 +1183,4 @@ export const discussion = { }, ], }, -} satisfies DiscussionResponse; +} satisfies DiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts index 77b0ea5d18f..7ecd5423991 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 { DiscussionSuccess } 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 DiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts index da50b586e6d..02c6fc38d71 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 { DiscussionSuccess } from '../../src/types/discussion'; export const discussionWithTwoComments = { status: 'ok', @@ -63,4 +63,4 @@ export const discussionWithTwoComments = { }, ], }, -} satisfies DiscussionResponse; +} satisfies DiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/short-discussion.ts b/dotcom-rendering/fixtures/manual/short-discussion.ts index e75b682d9fc..b48f4bfe782 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 { DiscussionSuccess } from '../../src/types/discussion'; export const shortDiscussion = { status: 'ok', @@ -59,4 +59,4 @@ export const shortDiscussion = { }, ], }, -} satisfies DiscussionResponse; +} satisfies DiscussionSuccess; 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; From e7d31f91dce2445459a9e3a133251195233beae8 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Fri, 26 Jan 2024 16:04:18 +0000 Subject: [PATCH 2/4] refactor(GetDiscussionSuccess): more explicit name --- .../fixtures/manual/discussion-no-top-comments.ts | 4 ++-- dotcom-rendering/fixtures/manual/discussion.ts | 4 ++-- dotcom-rendering/fixtures/manual/discussionWithNoComments.ts | 4 ++-- dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts | 4 ++-- dotcom-rendering/fixtures/manual/short-discussion.ts | 4 ++-- dotcom-rendering/src/lib/discussionApi.tsx | 4 ++-- dotcom-rendering/src/types/discussion.ts | 2 +- 7 files changed, 13 insertions(+), 13 deletions(-) diff --git a/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts b/dotcom-rendering/fixtures/manual/discussion-no-top-comments.ts index 4e3812d85ce..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 { DiscussionSuccess } 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 DiscussionSuccess; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussion.ts b/dotcom-rendering/fixtures/manual/discussion.ts index 24494632b3c..7b2ecc496c8 100644 --- a/dotcom-rendering/fixtures/manual/discussion.ts +++ b/dotcom-rendering/fixtures/manual/discussion.ts @@ -1,4 +1,4 @@ -import type { DiscussionSuccess } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussion = { status: 'ok', @@ -1183,4 +1183,4 @@ export const discussion = { }, ], }, -} satisfies DiscussionSuccess; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts index 7ecd5423991..87d420fd255 100644 --- a/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts +++ b/dotcom-rendering/fixtures/manual/discussionWithNoComments.ts @@ -1,4 +1,4 @@ -import type { DiscussionSuccess } 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 DiscussionSuccess; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts index 02c6fc38d71..d56c428ae56 100644 --- a/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts +++ b/dotcom-rendering/fixtures/manual/discussionWithTwoComments.ts @@ -1,4 +1,4 @@ -import type { DiscussionSuccess } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const discussionWithTwoComments = { status: 'ok', @@ -63,4 +63,4 @@ export const discussionWithTwoComments = { }, ], }, -} satisfies DiscussionSuccess; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/fixtures/manual/short-discussion.ts b/dotcom-rendering/fixtures/manual/short-discussion.ts index b48f4bfe782..5a146683fa0 100644 --- a/dotcom-rendering/fixtures/manual/short-discussion.ts +++ b/dotcom-rendering/fixtures/manual/short-discussion.ts @@ -1,4 +1,4 @@ -import type { DiscussionSuccess } from '../../src/types/discussion'; +import type { GetDiscussionSuccess } from '../../src/types/discussion'; export const shortDiscussion = { status: 'ok', @@ -59,4 +59,4 @@ export const shortDiscussion = { }, ], }, -} satisfies DiscussionSuccess; +} satisfies GetDiscussionSuccess; diff --git a/dotcom-rendering/src/lib/discussionApi.tsx b/dotcom-rendering/src/lib/discussionApi.tsx index 5287ba572fa..e90827fd6b7 100644 --- a/dotcom-rendering/src/lib/discussionApi.tsx +++ b/dotcom-rendering/src/lib/discussionApi.tsx @@ -5,7 +5,7 @@ import type { CommentResponse, CommentType, DiscussionOptions, - DiscussionSuccess, + GetDiscussionSuccess, OrderByType, ThreadsType, UserNameResponse, @@ -69,7 +69,7 @@ export const getDiscussion = async ( threads: ThreadsType; page: number; }, -): Promise> => { +): Promise> => { const apiOpts: DiscussionOptions = { ...defaultParams, ...{ diff --git a/dotcom-rendering/src/types/discussion.ts b/dotcom-rendering/src/types/discussion.ts index 8cf1966094c..e13cbe017c8 100644 --- a/dotcom-rendering/src/types/discussion.ts +++ b/dotcom-rendering/src/types/discussion.ts @@ -235,7 +235,7 @@ const discussionApiErrorSchema = object({ errorCode: optional(string()), }); -export type DiscussionSuccess = { +export type GetDiscussionSuccess = { status: 'ok'; currentPage: number; pages: number; From 52e5c6ee2fb2f898d391743de09b0b1d4e7ed125 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Fri, 26 Jan 2024 16:04:43 +0000 Subject: [PATCH 3/4] feat(JSON): wrap in Result type --- dotcom-rendering/src/lib/discussionApi.tsx | 17 ++++++----------- dotcom-rendering/src/lib/json.ts | 21 +++++++++++++++++++++ 2 files changed, 27 insertions(+), 11 deletions(-) create mode 100644 dotcom-rendering/src/lib/json.ts diff --git a/dotcom-rendering/src/lib/discussionApi.tsx b/dotcom-rendering/src/lib/discussionApi.tsx index e90827fd6b7..7d48706126f 100644 --- a/dotcom-rendering/src/lib/discussionApi.tsx +++ b/dotcom-rendering/src/lib/discussionApi.tsx @@ -13,6 +13,7 @@ import type { 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 = { @@ -58,7 +59,7 @@ const objAsParams = (obj: any): string => { return '?' + params; }; -type GetDiscussionError = 'Parsing error' | 'ApiError' | 'NetworkError'; +type GetDiscussionError = 'ParsingError' | 'ApiError' | 'NetworkError'; //todo: figure out the different return types and consider error handling export const getDiscussion = async ( @@ -89,19 +90,13 @@ export const getDiscussion = async ( const url = joinUrl(options.baseUrl, 'discussion', shortUrl) + params; - const json = (await fetch(url, { - headers: { - ...options.headers, - }, - }) - .then((r) => r.json()) - .catch(() => undefined)) as unknown; + const jsonResult = await fetchJSON(url, { headers: options.headers }); - if (!json) return { kind: 'error', error: 'NetworkError' }; + if (jsonResult.kind === 'error') return jsonResult; - const result = safeParse(discussionApiResponseSchema, json); + const result = safeParse(discussionApiResponseSchema, jsonResult.value); if (!result.success) { - return { kind: 'error', error: 'Parsing error' }; + return { kind: 'error', error: 'ParsingError' }; } if ( result.output.status === 'error' && 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' }; + } +}; From 90459cf0106885be3ecfc8a942f4cfc7038bc971 Mon Sep 17 00:00:00 2001 From: Max Duval Date: Fri, 26 Jan 2024 16:11:02 +0000 Subject: [PATCH 4/4] fix: reuse GetDiscussionSuccess --- .../components/DiscussionMeta.importable.tsx | 25 ++----------------- 1 file changed, 2 insertions(+), 23 deletions(-) diff --git a/dotcom-rendering/src/components/DiscussionMeta.importable.tsx b/dotcom-rendering/src/components/DiscussionMeta.importable.tsx index d0ac7d1cbb6..1442e35d747 100644 --- a/dotcom-rendering/src/components/DiscussionMeta.importable.tsx +++ b/dotcom-rendering/src/components/DiscussionMeta.importable.tsx @@ -3,6 +3,7 @@ import { getOptionsHeadersWithOkta } from '../lib/identity'; import { useApi } from '../lib/useApi'; import { useAuthStatus } from '../lib/useAuthStatus'; import { useCommentCount } from '../lib/useCommentCount'; +import type { GetDiscussionSuccess } from '../types/discussion'; import { SignedInAs } from './SignedInAs'; type Props = { @@ -11,28 +12,6 @@ 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, @@ -41,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