From 8b0c9cd0e1addc30a0ed60e169ca2ae7a82cc540 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 12:06:30 +0100 Subject: [PATCH 01/12] Allow running processes in background --- packages/cli-kit/src/public/node/system.ts | 12 ++++++++++-- packages/features/lib/system.ts | 1 + 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/public/node/system.ts b/packages/cli-kit/src/public/node/system.ts index fe77bdb0600..5d7d07b9322 100644 --- a/packages/cli-kit/src/public/node/system.ts +++ b/packages/cli-kit/src/public/node/system.ts @@ -20,6 +20,7 @@ export interface ExecOptions { signal?: AbortSignal // Custom handler if process exits with a non-zero code externalErrorHandler?: (error: unknown) => Promise + background?: boolean } /** @@ -54,6 +55,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}) } @@ -104,16 +110,18 @@ function buildExec(command: string, args: string[], options?: ExecOptions): Exec env, cwd: options?.cwd, 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: ${options?.cwd ?? cwd()} `) diff --git a/packages/features/lib/system.ts b/packages/features/lib/system.ts index e7f92b2b6e7..0b2022db524 100644 --- a/packages/features/lib/system.ts +++ b/packages/features/lib/system.ts @@ -5,6 +5,7 @@ import {ExecaChildProcess, execa} from 'execa' export interface ExecOptions { cwd?: string env?: NodeJS.ProcessEnv + detached?: boolean } /** From 14fbd5f1348b335d8ce92a02e4bc2d8fbad714ae Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 12:06:57 +0100 Subject: [PATCH 02/12] Fetch notifications in background and save in cache --- packages/cli-kit/bin/fetch-notifications.js | 21 +++++++++++++++++++ .../cli-kit/src/public/node/hooks/postrun.ts | 5 ++++- 2 files changed, 25 insertions(+), 1 deletion(-) create mode 100644 packages/cli-kit/bin/fetch-notifications.js diff --git a/packages/cli-kit/bin/fetch-notifications.js b/packages/cli-kit/bin/fetch-notifications.js new file mode 100644 index 00000000000..a8c75deb2e9 --- /dev/null +++ b/packages/cli-kit/bin/fetch-notifications.js @@ -0,0 +1,21 @@ +import fetch from 'node-fetch' +import {notificationsUrl} from '../dist/public/node/notifications-system.js' +import {cacheStore} from '../dist/private/node/conf-store.js' + +async function fetchNotifications() { + try { + const url = notificationsUrl() + const response = await fetch(url, {signal: AbortSignal.timeout(3 * 1000)}) + if (response.status === 200) { + cacheStore(`notifications-${url}`, await response.text()) + console.log(`Notifications from ${url} cached successfully`) + } + } catch (error) { + const errorMessage = `Error fetching notifications: ${error.message}` + console.error(errorMessage) + const {sendErrorToBugsnag} = await import('../dist/public/node/error-handler.js') + await sendErrorToBugsnag(errorMessage, 'unexpected_error') + } +} + +fetchNotifications() diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 5a95ecd59fc..09ba7aee78f 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -3,12 +3,16 @@ 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 {exec} from '../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'}) + await exec('node', ['./packages/cli-kit/bin/fetch-notifications.js'], { + background: true, + }) deprecationsHook(Command) const command = Command.id.replace(/:/g, ' ') @@ -28,7 +32,6 @@ async function detectStopCommand(commandClass: Command.Class | typeof BaseComman const {commandStartOptions} = metadata.getAllSensitiveMetadata() await metadata.addSensitiveMetadata(() => ({ commandStartOptions: { - // eslint-disable-next-line @typescript-eslint/no-non-null-assertion ...commandStartOptions!, startTime: currentTime, startCommand: stopCommand, From 9c2307a9e203c0352d7207a46e5e988ee14256fb Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 13:27:25 +0100 Subject: [PATCH 03/12] Do not fetch notifications from notifications-system, but only read from the cache --- .../public/node/notifications-system.test.ts | 16 ++++----- .../src/public/node/notifications-system.ts | 36 +++++++------------ 2 files changed, 21 insertions(+), 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 index c7b44ec34ce..94ff2cdcdee 100644 --- 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 index 8ca472a5d82..b73f1c92dc0 100644 --- 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 { - 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 { - 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. * From 774b67a262f004da2fea202e08553200f06679e0 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 13:28:06 +0100 Subject: [PATCH 04/12] Add changeset --- .changeset/violet-carrots-argue.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/violet-carrots-argue.md 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 From 4947e94c02cceb914796f19db504f1e4d6962e59 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 13:50:01 +0100 Subject: [PATCH 05/12] Fix linter --- packages/cli-kit/src/public/node/hooks/postrun.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 09ba7aee78f..792f90c6cab 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -30,9 +30,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!, + ...commandStartOptions, startTime: currentTime, startCommand: stopCommand, }, From 7a1c90c56e53c574ea693a90c2d2f04f1d5c1f75 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Wed, 4 Dec 2024 14:05:59 +0100 Subject: [PATCH 06/12] Undo unneeded change --- packages/features/lib/system.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/features/lib/system.ts b/packages/features/lib/system.ts index 0b2022db524..e7f92b2b6e7 100644 --- a/packages/features/lib/system.ts +++ b/packages/features/lib/system.ts @@ -5,7 +5,6 @@ import {ExecaChildProcess, execa} from 'execa' export interface ExecOptions { cwd?: string env?: NodeJS.ProcessEnv - detached?: boolean } /** From 276b93cc98cf7b43ac17521623376d53d02eb7c5 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Thu, 5 Dec 2024 15:33:06 +0100 Subject: [PATCH 07/12] Fix bundle issue --- .../cli-kit/assets/fetch-notifications.js | 25 +++++++++++++++++++ packages/cli-kit/bin/fetch-notifications.js | 21 ---------------- .../cli-kit/src/public/node/hooks/postrun.ts | 4 +-- .../src/public/node/notifications-system.ts | 2 +- 4 files changed, 27 insertions(+), 25 deletions(-) create mode 100644 packages/cli-kit/assets/fetch-notifications.js delete mode 100644 packages/cli-kit/bin/fetch-notifications.js diff --git a/packages/cli-kit/assets/fetch-notifications.js b/packages/cli-kit/assets/fetch-notifications.js new file mode 100644 index 00000000000..339c7252925 --- /dev/null +++ b/packages/cli-kit/assets/fetch-notifications.js @@ -0,0 +1,25 @@ +import fetch from 'node-fetch' +import Conf from 'conf' + +async function fetchNotifications() { + try { + const url = process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? 'https://cdn.shopify.com/static/cli/notifications.json' + const response = await fetch(url, {signal: AbortSignal.timeout(3 * 1000)}) + if (response.status === 200) { + storeNotifications(url, await response.text()) + console.log(`Notifications from ${url} cached successfully`) + } + } catch (error) { + const errorMessage = `Error fetching notifications: ${error.message}` + console.error(errorMessage) + } +} + +function storeNotifications(url, notifications) { + const config = new Conf({projectName: 'shopify-cli-kit'}) + const cache = config.get('cache') || {} + cache[`notifications-${url}`] = {value: notifications, timestamp: Date.now()} + config.set('cache', cache) +} + +await fetchNotifications() diff --git a/packages/cli-kit/bin/fetch-notifications.js b/packages/cli-kit/bin/fetch-notifications.js deleted file mode 100644 index a8c75deb2e9..00000000000 --- a/packages/cli-kit/bin/fetch-notifications.js +++ /dev/null @@ -1,21 +0,0 @@ -import fetch from 'node-fetch' -import {notificationsUrl} from '../dist/public/node/notifications-system.js' -import {cacheStore} from '../dist/private/node/conf-store.js' - -async function fetchNotifications() { - try { - const url = notificationsUrl() - const response = await fetch(url, {signal: AbortSignal.timeout(3 * 1000)}) - if (response.status === 200) { - cacheStore(`notifications-${url}`, await response.text()) - console.log(`Notifications from ${url} cached successfully`) - } - } catch (error) { - const errorMessage = `Error fetching notifications: ${error.message}` - console.error(errorMessage) - const {sendErrorToBugsnag} = await import('../dist/public/node/error-handler.js') - await sendErrorToBugsnag(errorMessage, 'unexpected_error') - } -} - -fetchNotifications() diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 792f90c6cab..3e51e92de6d 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -10,9 +10,7 @@ import {Command, Hook} from '@oclif/core' export const hook: Hook.Postrun = async ({config, Command}) => { await detectStopCommand(Command as unknown as typeof Command) await reportAnalyticsEvent({config, exitMode: 'ok'}) - await exec('node', ['./packages/cli-kit/bin/fetch-notifications.js'], { - background: true, - }) + await exec('node', [require.resolve('@shopify/cli-kit/assets/fetch-notifications.js')], {background: true}) deprecationsHook(Command) const command = Command.id.replace(/:/g, ' ') diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts index b73f1c92dc0..3aab0051299 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -112,7 +112,7 @@ 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, that is updated in the background from the fetch-notifications.json script. * * @returns A Notifications object. */ From 98e144d8bcb2af25f169439925a2b66e6f39735b Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Mon, 16 Dec 2024 11:35:54 +0100 Subject: [PATCH 08/12] Fetch notifications as part of the list command and run that in background --- .../cli-kit/assets/fetch-notifications.js | 25 ---------- .../cli-kit/src/public/node/hooks/postrun.ts | 4 +- .../src/public/node/notifications-system.ts | 50 ++++++++++++++++--- .../cli/services/commands/notifications.ts | 5 +- 4 files changed, 47 insertions(+), 37 deletions(-) delete mode 100644 packages/cli-kit/assets/fetch-notifications.js diff --git a/packages/cli-kit/assets/fetch-notifications.js b/packages/cli-kit/assets/fetch-notifications.js deleted file mode 100644 index 339c7252925..00000000000 --- a/packages/cli-kit/assets/fetch-notifications.js +++ /dev/null @@ -1,25 +0,0 @@ -import fetch from 'node-fetch' -import Conf from 'conf' - -async function fetchNotifications() { - try { - const url = process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? 'https://cdn.shopify.com/static/cli/notifications.json' - const response = await fetch(url, {signal: AbortSignal.timeout(3 * 1000)}) - if (response.status === 200) { - storeNotifications(url, await response.text()) - console.log(`Notifications from ${url} cached successfully`) - } - } catch (error) { - const errorMessage = `Error fetching notifications: ${error.message}` - console.error(errorMessage) - } -} - -function storeNotifications(url, notifications) { - const config = new Conf({projectName: 'shopify-cli-kit'}) - const cache = config.get('cache') || {} - cache[`notifications-${url}`] = {value: notifications, timestamp: Date.now()} - config.set('cache', cache) -} - -await fetchNotifications() diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 3e51e92de6d..8fe0a09ea12 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -3,14 +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 {exec} from '../system.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'}) - await exec('node', [require.resolve('@shopify/cli-kit/assets/fetch-notifications.js')], {background: true}) + fetchNotificationsInBackground() deprecationsHook(Command) const command = Command.id.replace(/:/g, ' ') diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts index 3aab0051299..ef33d41be91 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -6,18 +6,15 @@ import {zod} from './schema.js' import {AbortSilentError} from './error.js' import {isTruthy} from './context/utilities.js' import {jsonOutputEnabled} from './environment.js' +import {exec} from './system.js' import {CLI_KIT_VERSION} from '../common/version.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 EMPTY_CACHE_MESSAGE = 'Cache is empty' -/** - * 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 { +function url(): string { return process.env.SHOPIFY_CLI_NOTIFICATIONS_URL ?? URL } @@ -112,18 +109,55 @@ async function renderNotifications(notifications: Notification[]) { } /** - * Get notifications list from cache, that is updated in the background from the fetch-notifications.json script. + * 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-${notificationsUrl()}` + const cacheKey: NotificationsKey = `notifications-${url()}` 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 the CDN and chache them. + * + * @returns A string with the notifications. + */ +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}`) + const rawNotifications = await response.text() + await cacheNotifications(rawNotifications) + const notifications: object = JSON.parse(rawNotifications) + return NotificationsSchema.parse(notifications) +} + +/** + * 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. + */ +export function fetchNotificationsInBackground(): void { + // eslint-disable-next-line no-void + void exec('shopify', ['notifications', 'list'], { + background: true, + env: {...process.env, SHOPIFY_CLI_NO_ANALYTICS: '1'}, + }) +} + /** * Filters notifications based on the version of the CLI. * 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), } From 01d75c943c3b1be1f80cbf919db796845127efcc Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Mon, 16 Dec 2024 12:40:56 +0100 Subject: [PATCH 09/12] Do not fetch notifications on hidden commands --- packages/cli-kit/src/public/node/hooks/postrun.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index 8fe0a09ea12..bf23d7d3b8d 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -10,7 +10,7 @@ import {Command, Hook} from '@oclif/core' export const hook: Hook.Postrun = async ({config, Command}) => { await detectStopCommand(Command as unknown as typeof Command) await reportAnalyticsEvent({config, exitMode: 'ok'}) - fetchNotificationsInBackground() + if (!Command.hidden) fetchNotificationsInBackground() deprecationsHook(Command) const command = Command.id.replace(/:/g, ' ') From 604a6bf3b6964ee2882b8a194ed03b81e90e1389 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Mon, 16 Dec 2024 12:58:55 +0100 Subject: [PATCH 10/12] Always run the Shopify command the same way as the current execution --- .../cli-kit/src/public/node/hooks/postrun.ts | 2 +- .../public/node/notifications-system.test.ts | 35 ++++++++++++++++++- .../src/public/node/notifications-system.ts | 19 +++++++--- 3 files changed, 49 insertions(+), 7 deletions(-) diff --git a/packages/cli-kit/src/public/node/hooks/postrun.ts b/packages/cli-kit/src/public/node/hooks/postrun.ts index bf23d7d3b8d..f534ba31e73 100644 --- a/packages/cli-kit/src/public/node/hooks/postrun.ts +++ b/packages/cli-kit/src/public/node/hooks/postrun.ts @@ -10,7 +10,7 @@ import {Command, Hook} from '@oclif/core' 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() + if (!Command.hidden) fetchNotificationsInBackground(Command.id) deprecationsHook(Command) const command = Command.id.replace(/:/g, ' ') 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 94ff2cdcdee..0ea722f2876 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 {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', @@ -415,3 +422,29 @@ 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']) + + // 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']) + + // 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']) + + // 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 ef33d41be91..083fb38db25 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -149,13 +149,22 @@ async function cacheNotifications(notifications: string): Promise { /** * 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. */ -export function fetchNotificationsInBackground(): void { +export function fetchNotificationsInBackground(currentCommand: string, argv = process.argv): void { + 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('shopify', ['notifications', 'list'], { - background: true, - env: {...process.env, SHOPIFY_CLI_NO_ANALYTICS: '1'}, - }) + void exec(command, args, {background: true, env: {...process.env, SHOPIFY_CLI_NO_ANALYTICS: '1'}}) } /** From 95f2ab3e42e3cea1137f8de26f528814a7b41803 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Mon, 16 Dec 2024 14:21:16 +0100 Subject: [PATCH 11/12] Skip fetching notifications in CI/tests --- .../public/node/notifications-system.test.ts | 10 +++++++--- .../src/public/node/notifications-system.ts | 17 ++++++++++++----- 2 files changed, 19 insertions(+), 8 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 index 0ea722f2876..f7ac74ba881 100644 --- a/packages/cli-kit/src/public/node/notifications-system.test.ts +++ b/packages/cli-kit/src/public/node/notifications-system.test.ts @@ -426,7 +426,7 @@ describe('showNotificationsIfNeeded', () => { describe('fetchNotificationsInBackground', () => { test('calls the expected Shopify binary for global installation', async () => { // Given / When - fetchNotificationsInBackground('theme:init', ['shopify', 'theme', 'init']) + fetchNotificationsInBackground('theme:init', ['shopify', 'theme', 'init'], {SHOPIFY_UNIT_TEST: 'false'}) // Then expect(exec).toHaveBeenCalledWith('shopify', ['notifications', 'list'], expect.anything()) @@ -434,7 +434,9 @@ describe('fetchNotificationsInBackground', () => { test('calls the expected Shopify binary for local installation', async () => { // Given / When - fetchNotificationsInBackground('theme:init', ['npm', 'run', 'shopify', 'theme', 'init']) + fetchNotificationsInBackground('theme:init', ['npm', 'run', 'shopify', 'theme', 'init'], { + SHOPIFY_UNIT_TEST: 'false', + }) // Then expect(exec).toHaveBeenCalledWith('npm', ['run', 'shopify', 'notifications', 'list'], expect.anything()) @@ -442,7 +444,9 @@ describe('fetchNotificationsInBackground', () => { test('calls the expected Shopify binary for dev environment', async () => { // Given / When - fetchNotificationsInBackground('theme:init', ['node', 'packages/cli/bin/dev.js', 'theme', 'init']) + 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 083fb38db25..1f513a1e401 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -5,8 +5,8 @@ import {outputDebug} from './output.js' import {zod} from './schema.js' import {AbortSilentError} from './error.js' import {isTruthy} from './context/utilities.js' -import {jsonOutputEnabled} from './environment.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 {fetch} from '@shopify/cli-kit/node/http' @@ -55,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() @@ -74,8 +74,8 @@ export async function showNotificationsIfNeeded( } } -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) } /** @@ -152,8 +152,15 @@ async function cacheNotifications(notifications: string): Promise { * * @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): void { +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 From 2375a3f8ababda3ec9d3c3cb4781aa08dfb3fb96 Mon Sep 17 00:00:00 2001 From: Gonzalo Riestra Date: Tue, 7 Jan 2025 10:27:46 +0100 Subject: [PATCH 12/12] Avoid caching wrong values --- packages/cli-kit/src/public/node/notifications-system.ts | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/cli-kit/src/public/node/notifications-system.ts b/packages/cli-kit/src/public/node/notifications-system.ts index 1f513a1e401..f0d59cf7819 100644 --- a/packages/cli-kit/src/public/node/notifications-system.ts +++ b/packages/cli-kit/src/public/node/notifications-system.ts @@ -131,9 +131,10 @@ export async function fetchNotifications(): Promise { 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() - await cacheNotifications(rawNotifications) const notifications: object = JSON.parse(rawNotifications) - return NotificationsSchema.parse(notifications) + const result = NotificationsSchema.parse(notifications) + await cacheNotifications(rawNotifications) + return result } /**