Skip to content

Commit

Permalink
fix: notification services controller - remove retries (#4531)
Browse files Browse the repository at this point in the history
## 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

<!--
If you're making any consumer-facing changes, list those changes here as
if you were updating a changelog, using the template below as a guide.

(CATEGORY is one of BREAKING, ADDED, CHANGED, DEPRECATED, REMOVED, or
FIXED. For security-related issues, follow the Security Advisory
process.)

Please take care to name the exact pieces of the API you've added or
changed (e.g. types, interfaces, functions, or methods).

If there are any breaking changes, make sure to offer a solution for
consumers to follow once they upgrade to the changes.

Finally, if you're only making changes to development scripts or tests,
you may replace the template below with "None".
-->

### `@metamask/notification-services-controller`

- **CHANGED**: removed internal retry logic as this is not necessary and
also expensive (extends promises)
- **<CATEGORY>**: 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
  • Loading branch information
Prithpal-Sooriya authored Jul 18, 2024
1 parent 7969752 commit 4bc736a
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 85 deletions.
Original file line number Diff line number Diff line change
@@ -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';
Expand Down Expand Up @@ -37,53 +36,30 @@ export type ContentfulResult = {
items?: TypeFeatureAnnouncement[];
};

const fetchFromContentful = async (
url: string,
retries = 3,
retryDelay = 1000,
): Promise<ContentfulResult | null> => {
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<FeatureAnnouncementRawNotification[]> => {
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<ImageFields | TypeExtensionLinkFields> =
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;
};

Expand All @@ -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,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import log from 'loglevel';
import { v4 as uuidv4 } from 'uuid';

import {
Expand Down Expand Up @@ -425,65 +424,20 @@ 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<Response> {
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.
*
* @param bearerToken - The JSON Web Token for authorization.
* @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<Body>(
bearerToken: string,
endpoint: string,
method: 'POST' | 'DELETE',
body: Body,
retries = 3,
retryDelay = 1000,
): Promise<Response> {
const options: RequestInit = {
method,
Expand All @@ -494,5 +448,5 @@ export async function makeApiCall<Body>(
body: JSON.stringify(body),
};

return fetchWithRetry(endpoint, options, retries, retryDelay);
return await fetch(endpoint, options);
}

0 comments on commit 4bc736a

Please sign in to comment.