From 4bc736ac66625b751514491b04aa905f108fcfc3 Mon Sep 17 00:00:00 2001 From: Prithpal Sooriya Date: Thu, 18 Jul 2024 15:46:37 +0100 Subject: [PATCH] fix: notification services controller - remove retries (#4531) ## Explanation These retries are not necessary, and also are performed on the UI. Also these retries at a controller level are introducing bottlenecks, as retrying can extend async calls making UI loading much longer. ## References [NOTIFY-860](https://consensyssoftware.atlassian.net/browse/NOTIFY-860) ## Changelog ### `@metamask/notification-services-controller` - **CHANGED**: removed internal retry logic as this is not necessary and also expensive (extends promises) - ****: Your change here ## Checklist - [x] I've updated the test suite for new or updated code as appropriate - [x] I've updated documentation (JSDoc, Markdown, etc.) for new or updated code as appropriate - [x] I've highlighted breaking changes using the "BREAKING" category above as appropriate --- .../services/feature-announcements.ts | 53 ++++++------------- .../utils/utils.ts | 48 +---------------- 2 files changed, 16 insertions(+), 85 deletions(-) diff --git a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts index 42d982e866..957ea79848 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/services/feature-announcements.ts @@ -1,6 +1,5 @@ import { documentToHtmlString } from '@contentful/rich-text-html-renderer'; -import type { Entry, Asset } from 'contentful'; -import log from 'loglevel'; +import type { Entry, Asset, EntryCollection } from 'contentful'; import { TRIGGER_TYPES } from '../constants/notification-schema'; import { processFeatureAnnouncement } from '../processors/process-feature-announcement'; @@ -37,53 +36,30 @@ export type ContentfulResult = { items?: TypeFeatureAnnouncement[]; }; -const fetchFromContentful = async ( - url: string, - retries = 3, - retryDelay = 1000, -): Promise => { - let lastError: Error | null = null; - - for (let i = 0; i < retries; i++) { - try { - const response = await fetch(url); - if (!response.ok) { - throw new Error(`Fetch failed with status: ${response.status}`); - } - return await response.json(); - } catch (error) { - if (error instanceof Error) { - lastError = error; - } - if (i < retries - 1) { - await new Promise((resolve) => setTimeout(resolve, retryDelay)); - } - } - } - - log.error( - `Error fetching from Contentful after ${retries} retries:`, - lastError, - ); - return null; -}; +const getFeatureAnnouncementUrl = (env: Env) => + FEATURE_ANNOUNCEMENT_URL.replace(DEFAULT_SPACE_ID, env.spaceId) + .replace(DEFAULT_ACCESS_TOKEN, env.accessToken) + .replace(DEFAULT_CLIENT_ID, env.platform); const fetchFeatureAnnouncementNotifications = async ( env: Env, ): Promise => { - const url = FEATURE_ANNOUNCEMENT_URL.replace(DEFAULT_SPACE_ID, env.spaceId) - .replace(DEFAULT_ACCESS_TOKEN, env.accessToken) - .replace(DEFAULT_CLIENT_ID, env.platform); - const data = await fetchFromContentful(url); + const url = getFeatureAnnouncementUrl(env); + + const data = await fetch(url) + .then((r) => r.json()) + .catch(() => null); if (!data) { return []; } const findIncludedItem = (sysId: string) => { + const typedData: EntryCollection = + data; const item = - data?.includes?.Entry?.find((i: Entry) => i?.sys?.id === sysId) || - data?.includes?.Asset?.find((i: Asset) => i?.sys?.id === sysId); + typedData?.includes?.Entry?.find((i: Entry) => i?.sys?.id === sysId) || + typedData?.includes?.Asset?.find((i: Asset) => i?.sys?.id === sysId); return item ? item?.fields : null; }; @@ -94,6 +70,7 @@ const fetchFeatureAnnouncementNotifications = async ( const imageFields = fields.image ? (findIncludedItem(fields.image.sys.id) as ImageFields['fields']) : undefined; + const extensionLinkFields = fields.extensionLink ? (findIncludedItem( fields.extensionLink.sys.id, diff --git a/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts b/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts index de03d12e12..6e77d17a6e 100644 --- a/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts +++ b/packages/notification-services-controller/src/NotificationServicesController/utils/utils.ts @@ -1,4 +1,3 @@ -import log from 'loglevel'; import { v4 as uuidv4 } from 'uuid'; import { @@ -425,47 +424,6 @@ export function toggleUserStorageTriggerStatus( return userStorage; } -/** - * Attempts to fetch a resource from the network, retrying the request up to a specified number of times - * in case of failure, with a delay between attempts. - * - * @param url - The resource URL. - * @param options - The options for the fetch request. - * @param retries - Maximum number of retry attempts. Defaults to 3. - * @param retryDelay - Delay between retry attempts in milliseconds. Defaults to 1000. - * @returns A Promise resolving to the Response object. - * @throws Will throw an error if the request fails after the specified number of retries. - */ -async function fetchWithRetry( - url: string, - options: RequestInit, - retries = 3, - retryDelay = 1000, -): Promise { - for (let attempt = 1; attempt <= retries; attempt++) { - try { - const response = await fetch(url, options); - if (!response.ok) { - throw new Error(`Fetch failed with status: ${response.status}`); - } - return response; - } catch (error) { - log.error(`Attempt ${attempt} failed for fetch:`, error); - if (attempt < retries) { - await new Promise((resolve) => setTimeout(resolve, retryDelay)); - } else { - throw new Error( - `Fetching failed after ${retries} retries. Last error: ${ - error instanceof Error ? error.message : 'Unknown error' - }`, - ); - } - } - } - - throw new Error('Unexpected error in fetchWithRetry'); -} - /** * Performs an API call with automatic retries on failure. * @@ -473,8 +431,6 @@ async function fetchWithRetry( * @param endpoint - The URL of the API endpoint to call. * @param method - The HTTP method ('POST' or 'DELETE'). * @param body - The body of the request. It should be an object that can be serialized to JSON. - * @param retries - The number of retry attempts in case of failure (default is 3). - * @param retryDelay - The delay between retries in milliseconds (default is 1000). * @returns A Promise that resolves to the response of the fetch request. */ export async function makeApiCall( @@ -482,8 +438,6 @@ export async function makeApiCall( endpoint: string, method: 'POST' | 'DELETE', body: Body, - retries = 3, - retryDelay = 1000, ): Promise { const options: RequestInit = { method, @@ -494,5 +448,5 @@ export async function makeApiCall( body: JSON.stringify(body), }; - return fetchWithRetry(endpoint, options, retries, retryDelay); + return await fetch(endpoint, options); }