diff --git a/.changeset/violet-carrots-argue.md b/.changeset/violet-carrots-argue.md new file mode 100644 index 00000000000..50c718abf09 --- /dev/null +++ b/.changeset/violet-carrots-argue.md @@ -0,0 +1,5 @@ +--- +'@shopify/cli-kit': patch +--- + +Fetch notifications in background diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 5a95ecd59fc..f534ba31e73 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/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, }, diff --git a/packages/cli-kit/src/public/node/notifications-system.test.ts b/packages/cli-kit/src/public/node/notifications-system.test.ts index c7b44ec34ce..f7ac74ba881 100644 --- a/packages/cli-kit/src/public/node/notifications-system.test.ts +++ b/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()) + }) +}) diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts index 8ca472a5d82..f0d59cf7819 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/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 { 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 { 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 { - outputDebug(`No cached notifications found. Fetching them...`) +export async function fetchNotifications(): Promise { + 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 { + 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'}}) } /** diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index 673fba6c422..66747f3f113 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/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 + 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 { 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} `) diff --git a/packages/cli/src/cli/services/commands/notifications.ts b/packages/cli/src/cli/services/commands/notifications.ts index 111780c73c1..aa712a7b466 100644 --- a/packages/cli/src/cli/services/commands/notifications.ts +++ b/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), }