Do not fetch notifications from notifications-system, but only read f…

…rom the cache
Shopify · Dec 4, 2024 · 9c2307a · 9c2307a
1 parent 14fbd5f
commit 9c2307a
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 31 deletions.
diff --git a/packages/cli-kit/src/public/node/notifications-system.test.ts b/packages/cli-kit/src/public/node/notifications-system.test.ts
@@ -1,7 +1,7 @@
 import {Notification, filterNotifications, showNotificationsIfNeeded} from './notifications-system.js'
 import {renderError, renderInfo, renderWarning} from './ui.js'
 import {sniffForJson} from './path.js'
-import {cacheRetrieve, cacheRetrieveOrRepopulate} from '../../private/node/conf-store.js'
+import {cacheRetrieve} from '../../private/node/conf-store.js'
 import {afterEach, describe, expect, test, vi} from 'vitest'
 
 vi.mock('./ui.js')
@@ -333,7 +333,7 @@ describe('showNotificationsIfNeeded', () => {
   test('an info notification triggers a renderInfo call', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})
@@ -345,7 +345,7 @@ describe('showNotificationsIfNeeded', () => {
   test('a warning notification triggers a renderWarning call', async () => {
     // Given
     const notifications = [warningNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})
@@ -357,7 +357,7 @@ describe('showNotificationsIfNeeded', () => {
   test('an error notification triggers a renderError call and throws an error', async () => {
     // Given
     const notifications = [errorNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await expect(showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})).rejects.toThrowError()
@@ -369,7 +369,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped on CI', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false', CI: 'true'})
@@ -381,7 +381,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped on tests', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'true'})
@@ -393,7 +393,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped when using --json flag', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
     vi.mocked(sniffForJson).mockReturnValue(true)
 
     // When
@@ -406,7 +406,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped when using SHOPIFY_FLAG_JSON', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
+    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false', SHOPIFY_FLAG_JSON: 'true'})

diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts
@@ -7,19 +7,17 @@ import {AbortSilentError} from './error.js'
 import {isTruthy} from './context/utilities.js'
 import {jsonOutputEnabled} from './environment.js'
 import {CLI_KIT_VERSION} from '../common/version.js'
-import {
-  NotificationKey,
-  NotificationsKey,
-  cacheRetrieve,
-  cacheRetrieveOrRepopulate,
-  cacheStore,
-} from '../../private/node/conf-store.js'
-import {fetch} from '@shopify/cli-kit/node/http'
+import {NotificationKey, NotificationsKey, cacheRetrieve, cacheStore} from '../../private/node/conf-store.js'
 
 const URL = 'https://cdn.shopify.com/static/cli/notifications.json'
-const CACHE_DURATION_IN_MS = 3600 * 1000
+const EMPTY_CACHE_MESSAGE = 'Cache is empty'
 
-function url(): string {
+/**
+ * Returns the URL to retrieve the notifications.
+ *
+ * @returns - The value from SHOPIFY_CLI_NOTIFICATIONS_URL or the default URL (https://cdn.shopify.com/static/cli/notifications.json).
+ */
+export function notificationsUrl(): string {
   return process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? URL
 }
 
@@ -72,6 +70,7 @@ export async function showNotificationsIfNeeded(
     if (error.message === 'abort') throw new AbortSilentError()
     const errorMessage = `Error retrieving notifications: ${error.message}`
     outputDebug(errorMessage)
+    if (error.message === EMPTY_CACHE_MESSAGE) return
     // This is very prone to becoming a circular dependency, so we import it dynamically
     const {sendErrorToBugsnag} = await import('./error-handler.js')
     await sendErrorToBugsnag(errorMessage, 'unexpected_error')
@@ -113,27 +112,18 @@ async function renderNotifications(notifications: Notification[]) {
 }
 
 /**
- * Get notifications list from cache (refreshed every hour) or fetch it if not present.
+ * Get notifications list from cache, that is updated in the background from bin/fetch-notifications.json.
  *
  * @returns A Notifications object.
  */
 export async function getNotifications(): Promise<Notifications> {
-  const cacheKey: NotificationsKey = `notifications-${url()}`
-  const rawNotifications = await cacheRetrieveOrRepopulate(cacheKey, fetchNotifications, CACHE_DURATION_IN_MS)
+  const cacheKey: NotificationsKey = `notifications-${notificationsUrl()}`
+  const rawNotifications = cacheRetrieve(cacheKey)?.value as unknown as string
+  if (!rawNotifications) throw new Error(EMPTY_CACHE_MESSAGE)
   const notifications: object = JSON.parse(rawNotifications)
   return NotificationsSchema.parse(notifications)
 }
 
-/**
- * Fetch notifications from GitHub.
- */
-async function fetchNotifications(): Promise<string> {
-  outputDebug(`No cached notifications found. Fetching them...`)
-  const response = await fetch(url(), {signal: AbortSignal.timeout(3 * 1000)})
-  if (response.status !== 200) throw new Error(`Failed to fetch notifications: ${response.statusText}`)
-  return response.text() as unknown as string
-}
-
 /**
  * Filters notifications based on the version of the CLI.
  *