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

Fetch notifications in background

.changeset/violet-carrots-argue.md

@@ -0,0 +1,5 @@
+---
+'@shopify/cli-kit': patch
+---
+
+Fetch notifications in background

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

@@ -3,12 +3,14 @@ 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, ' ')
@@ -26,10 +28,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: {
-          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-          ...commandStartOptions!,
+          ...commandStartOptions,
           startTime: currentTime,
           startCommand: stopCommand,
         },

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

@@ -1,12 +1,19 @@
-import {Notification, filterNotifications, showNotificationsIfNeeded} from './notifications-system.js'
+import {
+  Notification,
+  fetchNotificationsInBackground,
+  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 {exec} from './system.js'
+import {cacheRetrieve} 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',
@@ -333,7 +340,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 +352,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 +364,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 +376,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 +388,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 +400,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 +413,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'})
@@ -415,3 +422,33 @@ 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.ts

@@ -5,19 +5,14 @@ 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,
-  cacheRetrieveOrRepopulate,
-  cacheStore,
-} from '../../private/node/conf-store.js'
+import {NotificationKey, NotificationsKey, cacheRetrieve, 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 CACHE_DURATION_IN_MS = 3600 * 1000
+const EMPTY_CACHE_MESSAGE = 'Cache is empty'
 
 function url(): string {
   return process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? URL
@@ -60,7 +55,7 @@ export async function showNotificationsIfNeeded(
   environment: NodeJS.ProcessEnv = process.env,
 ): Promise<void> {
   try {
-    if (skipNotifications(environment)) return
+    if (skipNotifications(environment) || jsonOutputEnabled(environment)) return
 
     const notifications = await getNotifications()
     const commandId = getCurrentCommandId()
@@ -72,14 +67,15 @@ 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): boolean {
-  return isTruthy(environment.CI) || isTruthy(environment.SHOPIFY_UNIT_TEST) || jsonOutputEnabled(environment)
+function skipNotifications(environment: NodeJS.ProcessEnv = process.env): boolean {
+  return isTruthy(environment.CI) || isTruthy(environment.SHOPIFY_UNIT_TEST)
 }
 
 /**
@@ -113,25 +109,70 @@ 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 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.
+ * Fetch notifications from the CDN and chache them.
+ *
+ * @returns A string with the notifications.
  */
-async function fetchNotifications(): Promise<string> {
-  outputDebug(`No cached notifications found. Fetching them...`)
+export async function fetchNotifications(): Promise<Notifications> {
+  outputDebug(`Fetching  notifications...`)
   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
+  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'}})
 }
 
 /**

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

@@ -21,6 +21,7 @@ export interface ExecOptions {
   signal?: AbortSignal
   // Custom handler if process exits with a non-zero code
   externalErrorHandler?: (error: unknown) => Promise<void>
+  background?: boolean
 }
 
 /**
@@ -55,6 +56,11 @@ 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})
   }
@@ -106,16 +112,18 @@ function buildExec(command: string, args: string[], options?: ExecOptions): Exec
     env,
     cwd: executionCwd,
     input: options?.input,
-    stdio: options?.stdio,
+    stdio: options?.background ? 'ignore' : 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:
+Running system process${options?.background ? ' in background' : ''}:
   · Command: ${command} ${args.join(' ')}
   · Working directory: ${executionCwd}
 `)

packages/cli/src/cli/services/commands/notifications.ts

@@ -6,6 +6,7 @@ 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'
@@ -95,7 +96,7 @@ export async function generate() {
 }
 
 export async function list() {
-  const notifications: Notifications = await getNotifications()
+  const notifications = await fetchNotifications()
 
   const columns: TableColumn<{type: string; title: string; message: string; filters: string}> = {
     type: {header: 'Type', color: 'dim'},
@@ -107,7 +108,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),
     }