Merge pull request #5234 from Shopify/revert-5020-fetch-notifications-in-background

Revert "Fetch notifications in background"

Author: gonzaloriestra

.changeset/violet-carrots-argue.md

@@ -3,14 +3,12 @@ import {reportAnalyticsEvent} from '../analytics.js'
 import {outputDebug} from '../../../public/node/output.js'
 import BaseCommand from '../base-command.js'
 import * as metadata from '../../../public/node/metadata.js'
-import {fetchNotificationsInBackground} from '../notifications-system.js'
 import {Command, Hook} from '@oclif/core'
 
 // This hook is called after each successful command run. More info: https://oclif.io/docs/hooks
 export const hook: Hook.Postrun = async ({config, Command}) => {
   await detectStopCommand(Command as unknown as typeof Command)
   await reportAnalyticsEvent({config, exitMode: 'ok'})
-  if (!Command.hidden) fetchNotificationsInBackground(Command.id)
   deprecationsHook(Command)
 
   const command = Command.id.replace(/:/g, ' ')
@@ -28,10 +26,10 @@ async function detectStopCommand(commandClass: Command.Class | typeof BaseComman
     const stopCommand = (commandClass as typeof BaseCommand).analyticsStopCommand()
     if (stopCommand) {
       const {commandStartOptions} = metadata.getAllSensitiveMetadata()
-      if (!commandStartOptions) return
       await metadata.addSensitiveMetadata(() => ({
         commandStartOptions: {
-          ...commandStartOptions,
+          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+          ...commandStartOptions!,
           startTime: currentTime,
           startCommand: stopCommand,
         },

packages/cli-kit/src/public/node/hooks/postrun.ts

@@ -1,19 +1,12 @@
-import {
-  Notification,
-  fetchNotificationsInBackground,
-  filterNotifications,
-  showNotificationsIfNeeded,
-} from './notifications-system.js'
+import {Notification, filterNotifications, showNotificationsIfNeeded} from './notifications-system.js'
 import {renderError, renderInfo, renderWarning} from './ui.js'
 import {sniffForJson} from './path.js'
-import {exec} from './system.js'
-import {cacheRetrieve} from '../../private/node/conf-store.js'
+import {cacheRetrieve, cacheRetrieveOrRepopulate} from '../../private/node/conf-store.js'
 import {afterEach, describe, expect, test, vi} from 'vitest'
 
 vi.mock('./ui.js')
 vi.mock('../../private/node/conf-store.js')
 vi.mock('./path.js')
-vi.mock('./system.js')
 
 const betweenVersins1and2: Notification = {
   id: 'betweenVersins1and2',
@@ -340,7 +333,7 @@ describe('showNotificationsIfNeeded', () => {
   test('an info notification triggers a renderInfo call', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})
@@ -352,7 +345,7 @@ describe('showNotificationsIfNeeded', () => {
   test('a warning notification triggers a renderWarning call', async () => {
     // Given
     const notifications = [warningNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})
@@ -364,7 +357,7 @@ describe('showNotificationsIfNeeded', () => {
   test('an error notification triggers a renderError call and throws an error', async () => {
     // Given
     const notifications = [errorNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await expect(showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false'})).rejects.toThrowError()
@@ -376,7 +369,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped on CI', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false', CI: 'true'})
@@ -388,7 +381,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped on tests', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'true'})
@@ -400,7 +393,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped when using --json flag', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
     vi.mocked(sniffForJson).mockReturnValue(true)
 
     // When
@@ -413,7 +406,7 @@ describe('showNotificationsIfNeeded', () => {
   test('notifications are skipped when using SHOPIFY_FLAG_JSON', async () => {
     // Given
     const notifications = [infoNotification]
-    vi.mocked(cacheRetrieve).mockReturnValue({value: JSON.stringify({notifications}), timestamp: 0})
+    vi.mocked(cacheRetrieveOrRepopulate).mockResolvedValue(JSON.stringify({notifications}))
 
     // When
     await showNotificationsIfNeeded(undefined, {SHOPIFY_UNIT_TEST: 'false', SHOPIFY_FLAG_JSON: 'true'})
@@ -422,33 +415,3 @@ describe('showNotificationsIfNeeded', () => {
     expect(renderInfo).not.toHaveBeenCalled()
   })
 })
-
-describe('fetchNotificationsInBackground', () => {
-  test('calls the expected Shopify binary for global installation', async () => {
-    // Given / When
-    fetchNotificationsInBackground('theme:init', ['shopify', 'theme', 'init'], {SHOPIFY_UNIT_TEST: 'false'})
-
-    // Then
-    expect(exec).toHaveBeenCalledWith('shopify', ['notifications', 'list'], expect.anything())
-  })
-
-  test('calls the expected Shopify binary for local installation', async () => {
-    // Given / When
-    fetchNotificationsInBackground('theme:init', ['npm', 'run', 'shopify', 'theme', 'init'], {
-      SHOPIFY_UNIT_TEST: 'false',
-    })
-
-    // Then
-    expect(exec).toHaveBeenCalledWith('npm', ['run', 'shopify', 'notifications', 'list'], expect.anything())
-  })
-
-  test('calls the expected Shopify binary for dev environment', async () => {
-    // Given / When
-    fetchNotificationsInBackground('theme:init', ['node', 'packages/cli/bin/dev.js', 'theme', 'init'], {
-      SHOPIFY_UNIT_TEST: 'false',
-    })
-
-    // Then
-    expect(exec).toHaveBeenCalledWith('node', ['packages/cli/bin/dev.js', 'notifications', 'list'], expect.anything())
-  })
-})

packages/cli-kit/src/public/node/notifications-system.test.ts

@@ -5,14 +5,19 @@ import {outputDebug} from './output.js'
 import {zod} from './schema.js'
 import {AbortSilentError} from './error.js'
 import {isTruthy} from './context/utilities.js'
-import {exec} from './system.js'
 import {jsonOutputEnabled} from './environment.js'
 import {CLI_KIT_VERSION} from '../common/version.js'
-import {NotificationKey, NotificationsKey, cacheRetrieve, cacheStore} from '../../private/node/conf-store.js'
+import {
+  NotificationKey,
+  NotificationsKey,
+  cacheRetrieve,
+  cacheRetrieveOrRepopulate,
+  cacheStore,
+} from '../../private/node/conf-store.js'
 import {fetch} from '@shopify/cli-kit/node/http'
 
 const URL = 'https://cdn.shopify.com/static/cli/notifications.json'
-const EMPTY_CACHE_MESSAGE = 'Cache is empty'
+const CACHE_DURATION_IN_MS = 3600 * 1000
 
 function url(): string {
   return process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? URL
@@ -55,7 +60,7 @@ export async function showNotificationsIfNeeded(
   environment: NodeJS.ProcessEnv = process.env,
 ): Promise<void> {
   try {
-    if (skipNotifications(environment) || jsonOutputEnabled(environment)) return
+    if (skipNotifications(environment)) return
 
     const notifications = await getNotifications()
     const commandId = getCurrentCommandId()
@@ -67,15 +72,14 @@ 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')
   }
 }
 
-function skipNotifications(environment: NodeJS.ProcessEnv = process.env): boolean {
-  return isTruthy(environment.CI) || isTruthy(environment.SHOPIFY_UNIT_TEST)
+function skipNotifications(environment: NodeJS.ProcessEnv): boolean {
+  return isTruthy(environment.CI) || isTruthy(environment.SHOPIFY_UNIT_TEST) || jsonOutputEnabled(environment)
 }
 
 /**
@@ -109,70 +113,25 @@ async function renderNotifications(notifications: Notification[]) {
 }
 
 /**
- * Get notifications list from cache, that is updated in the background from bin/fetch-notifications.json.
+ * Get notifications list from cache (refreshed every hour) or fetch it if not present.
  *
  * @returns A Notifications object.
  */
 export async function getNotifications(): Promise<Notifications> {
   const cacheKey: NotificationsKey = `notifications-${url()}`
-  const rawNotifications = cacheRetrieve(cacheKey)?.value as unknown as string
-  if (!rawNotifications) throw new Error(EMPTY_CACHE_MESSAGE)
+  const rawNotifications = await cacheRetrieveOrRepopulate(cacheKey, fetchNotifications, CACHE_DURATION_IN_MS)
   const notifications: object = JSON.parse(rawNotifications)
   return NotificationsSchema.parse(notifications)
 }
 
 /**
- * Fetch notifications from the CDN and chache them.
- *
- * @returns A string with the notifications.
+ * Fetch notifications from GitHub.
  */
-export async function fetchNotifications(): Promise<Notifications> {
-  outputDebug(`Fetching  notifications...`)
+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}`)
-  const rawNotifications = await response.text()
-  const notifications: object = JSON.parse(rawNotifications)
-  const result = NotificationsSchema.parse(notifications)
-  await cacheNotifications(rawNotifications)
-  return result
-}
-
-/**
- * Store the notifications in the cache.
- *
- * @param notifications - String with the notifications to cache.
- * @returns A Notifications object.
- */
-async function cacheNotifications(notifications: string): Promise<void> {
-  cacheStore(`notifications-${url()}`, notifications)
-  outputDebug(`Notifications from ${url()} stored in the cache`)
-}
-
-/**
- * Fetch notifications in background as a detached process.
- *
- * @param currentCommand - The current Shopify command being run.
- * @param argv - The arguments passed to the current process.
- * @param environment - Process environment variables.
- */
-export function fetchNotificationsInBackground(
-  currentCommand: string,
-  argv = process.argv,
-  environment: NodeJS.ProcessEnv = process.env,
-): void {
-  if (skipNotifications(environment)) return
-
-  let command = 'shopify'
-  const args = ['notifications', 'list']
-  // Run the Shopify command the same way as the current execution when it's not the global installation
-  if (argv[0] && argv[0] !== 'shopify') {
-    command = argv[0]
-    const indexValue = currentCommand.split(':')[0] ?? ''
-    const index = argv.indexOf(indexValue)
-    if (index > 0) args.unshift(...argv.slice(1, index))
-  }
-  // eslint-disable-next-line no-void
-  void exec(command, args, {background: true, env: {...process.env, SHOPIFY_CLI_NO_ANALYTICS: '1'}})
+  return response.text() as unknown as string
 }
 
 /**

packages/cli-kit/src/public/node/notifications-system.ts

@@ -21,7 +21,6 @@ export interface ExecOptions {
   signal?: AbortSignal
   // Custom handler if process exits with a non-zero code
   externalErrorHandler?: (error: unknown) => Promise<void>
-  background?: boolean
 }
 
 /**
@@ -56,11 +55,6 @@ export async function captureOutput(command: string, args: string[], options?: E
  */
 export async function exec(command: string, args: string[], options?: ExecOptions): Promise<void> {
   const commandProcess = buildExec(command, args, options)
-
-  if (options?.background) {
-    commandProcess.unref()
-  }
-
   if (options?.stderr && options.stderr !== 'inherit') {
     commandProcess.stderr?.pipe(options.stderr, {end: false})
   }
@@ -112,18 +106,16 @@ function buildExec(command: string, args: string[], options?: ExecOptions): Exec
     env,
     cwd: executionCwd,
     input: options?.input,
-    stdio: options?.background ? 'ignore' : options?.stdio,
+    stdio: options?.stdio,
     stdin: options?.stdin,
     stdout: options?.stdout === 'inherit' ? 'inherit' : undefined,
     stderr: options?.stderr === 'inherit' ? 'inherit' : undefined,
     // Setting this to false makes it possible to kill the main process
     // and all its sub-processes with Ctrl+C on Windows
     windowsHide: false,
-    detached: options?.background,
-    cleanup: !options?.background,
   })
   outputDebug(`
-Running system process${options?.background ? ' in background' : ''}:
+Running system process:
   · Command: ${command} ${args.join(' ')}
   · Working directory: ${executionCwd}
 `)

packages/cli-kit/src/public/node/system.ts

@@ -6,7 +6,6 @@ import {
   Notification,
   stringifyFilters,
   getNotifications,
-  fetchNotifications,
 } from '@shopify/cli-kit/node/notifications-system'
 import {outputInfo} from '@shopify/cli-kit/node/output'
 import {renderSelectPrompt, renderTextPrompt, renderSuccess, renderTable, TableColumn} from '@shopify/cli-kit/node/ui'
@@ -96,7 +95,7 @@ export async function generate() {
 }
 
 export async function list() {
-  const notifications = await fetchNotifications()
+  const notifications: Notifications = await getNotifications()
 
   const columns: TableColumn<{type: string; title: string; message: string; filters: string}> = {
     type: {header: 'Type', color: 'dim'},
@@ -108,7 +107,7 @@ export async function list() {
   const rows = notifications.notifications.map((notification: Notification) => {
     return {
       type: notification.type,
-      title: notification.title ?? '',
+      title: notification.title || '',
       message: notification.message,
       filters: stringifyFilters(notification),
     }