From 458a98b5824fcbd3afe5949dad9268cf626f3c0f Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Tue, 8 Apr 2025 10:39:05 -0400 Subject: [PATCH 01/12] Improvements to development using the --use-localhost flag There are 2 improvements: 1. --use-localhost will always try to use the same port if it is available. 2. A new --localhost-port flag has been added to specify a port. If neither the default port, or the one specified using --localhost-port is not available, we show a warning to the developer and choose a random available port. --- packages/app/src/cli/commands/app/dev.ts | 36 +++++++++++++++++-- packages/app/src/cli/constants.ts | 1 + packages/app/src/cli/services/dev.ts | 3 ++ .../dev/processes/setup-dev-processes.ts | 8 +++-- packages/app/src/cli/services/dev/urls.ts | 3 +- 5 files changed, 44 insertions(+), 7 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index a0a5c13b32..97a363c70a 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -7,11 +7,13 @@ import {linkedAppContext} from '../../services/app-context.js' import {storeContext} from '../../services/store-context.js' import {generateCertificate} from '../../utilities/mkcert.js' import {generateCertificatePrompt} from '../../prompts/dev.js' +import {ports} from '../../constants.js' import {Flags} from '@oclif/core' import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' import {globalFlags} from '@shopify/cli-kit/node/cli' import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' -import {renderInfo} from '@shopify/cli-kit/node/ui' +import {renderInfo, renderWarning} from '@shopify/cli-kit/node/ui' +import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' export default class Dev extends AppCommand { static summary = 'Run the app.' @@ -91,11 +93,17 @@ If you're using the Ruby app template, then you need to complete the following s 'use-localhost': Flags.boolean({ hidden: true, description: - "Service entry point will listen to localhost. A tunnel won't be used. Will work for testing many app features, but not Webhooks, Flow Action, App Proxy or POS", + "Service entry point will listen to localhost. A tunnel won't be used. Will work for testing many app features, but not those that directly invoke your app (E.g: Webhooks)", env: 'SHOPIFY_FLAG_USE_LOCALHOST', default: false, exclusive: ['tunnel-url'], }), + 'localhost-port': Flags.integer({ + hidden: true, + description: 'Port to use for localhost. Only applicable when --use-localhost is specified.', + env: 'SHOPIFY_FLAG_LOCALHOST_PORT', + dependsOn: ['use-localhost'], + }), theme: Flags.string({ hidden: false, char: 't', @@ -144,8 +152,16 @@ If you're using the Ruby app template, then you need to complete the following s if (flags['use-localhost']) { tunnelType = 'use-localhost' + + // Check if a port was specified with --localhost-port + const requestedPort = flags['localhost-port'] ?? ports.localhost + const actualPort = (await checkPortAvailability(requestedPort)) + ? requestedPort + : await getAvailableTCPPort(ports.localhost) + tunnel = { mode: 'no-tunnel-use-localhost', + port: actualPort, provideCertificate: async (appDirectory) => { renderInfo({ headline: 'Localhost-based development is in developer preview.', @@ -160,6 +176,21 @@ If you're using the Ruby app template, then you need to complete the following s }, }) + if (requestedPort && requestedPort !== actualPort) { + renderWarning({ + headline: [ + 'A random port will be used for localhost because', + {command: `${requestedPort}`}, + 'is not available.', + ], + body: [ + 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', + {command: '--localhost-port PORT'}, + 'flag.', + ], + }) + } + return generateCertificate({ appDirectory, onRequiresConfirmation: generateCertificatePrompt, @@ -190,7 +221,6 @@ If you're using the Ruby app template, then you need to complete the following s forceRelink: flags.reset, userProvidedConfigName: flags.config, }) - const store = await storeContext({ appContextResult, storeFqdn: flags.store, diff --git a/packages/app/src/cli/constants.ts b/packages/app/src/cli/constants.ts index eacb2e5d6f..6922035558 100644 --- a/packages/app/src/cli/constants.ts +++ b/packages/app/src/cli/constants.ts @@ -37,6 +37,7 @@ export const blocks = { export const ports = { graphiql: 3457, + localhost: 3568, } as const export const EsbuildEnvVarRegex = /^([a-zA-Z_$])([a-zA-Z0-9_$])*$/ diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 0da66072b9..c865860fcc 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -48,6 +48,7 @@ import {AbortError} from '@shopify/cli-kit/node/error' interface NoTunnel { mode: 'no-tunnel-use-localhost' + port?: number provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> } @@ -326,6 +327,7 @@ async function setupNetworkingOptions( noTunnelUseLocalhost: tunnelOptions.mode === 'no-tunnel-use-localhost', tunnelUrl: tunnelOptions.mode === 'provided' ? tunnelOptions.url : undefined, tunnelClient, + port: tunnelOptions.mode === 'no-tunnel-use-localhost' ? tunnelOptions.port : undefined, }), getBackendPort() ?? backendConfig?.configuration.port ?? getAvailableTCPPort(), getURLs(remoteAppConfig), @@ -348,6 +350,7 @@ async function setupNetworkingOptions( key: keyContent, cert: certContent, certPath, + port: tunnelOptions.port, } } diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts index a5ab9c50a5..fb550798a8 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts @@ -269,10 +269,12 @@ export const startProxyServer: DevProcessFunction<{ rules: {[key: string]: string} localhostCert?: LocalhostCert }> = async ({abortSignal, stdout}, {port, rules, localhostCert}) => { - const {server} = await getProxyingWebServer(rules, abortSignal, localhostCert) + const {server, port: certPort} = await getProxyingWebServer(rules, abortSignal, localhostCert) + // Use the port from the certificate if available, otherwise use the provided port + const actualPort = certPort ?? port outputInfo( - `Proxy server started on port ${port} ${localhostCert ? `with certificate ${localhostCert.certPath}` : ''}`, + `Proxy server started on port ${actualPort} ${localhostCert ? `with certificate ${localhostCert.certPath}` : ''}`, stdout, ) - await server.listen(port) + await server.listen(actualPort) } diff --git a/packages/app/src/cli/services/dev/urls.ts b/packages/app/src/cli/services/dev/urls.ts index 8567869ed2..65ffed4d2f 100644 --- a/packages/app/src/cli/services/dev/urls.ts +++ b/packages/app/src/cli/services/dev/urls.ts @@ -32,6 +32,7 @@ export interface FrontendURLOptions { noTunnelUseLocalhost: boolean tunnelUrl?: string tunnelClient: TunnelClient | undefined + port?: number } interface FrontendURLResult { @@ -99,7 +100,7 @@ export async function generateFrontendURL(options: FrontendURLOptions): Promise< } if (options.noTunnelUseLocalhost) { - frontendPort = await getAvailableTCPPort() + frontendPort = options.port ?? (await getAvailableTCPPort()) frontendUrl = 'https://localhost' } else if (options.tunnelClient) { const url = await pollTunnelURL(options.tunnelClient) From 3a7d7b88befa8860a481ab67885e516e6c58d418 Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Wed, 9 Apr 2025 14:39:08 -0400 Subject: [PATCH 02/12] Move tunnel logic to setup the tunnel out of the command and into the service. Add tests --- packages/app/src/cli/commands/app/dev.ts | 73 +------ packages/app/src/cli/services/dev.test.ts | 182 +++++++++++++++++- packages/app/src/cli/services/dev.ts | 93 +++++++-- .../dev/processes/setup-dev-processes.ts | 8 +- 4 files changed, 272 insertions(+), 84 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 97a363c70a..f4264c3d15 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -1,19 +1,14 @@ import {appFlags} from '../../flags.js' -import {dev, DevOptions, TunnelMode} from '../../services/dev.js' +import {dev, DevOptions, getTunnelMode} from '../../services/dev.js' import {showApiKeyDeprecationWarning} from '../../prompts/deprecation-warnings.js' import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppCommand, {AppCommandOutput} from '../../utilities/app-command.js' import {linkedAppContext} from '../../services/app-context.js' import {storeContext} from '../../services/store-context.js' -import {generateCertificate} from '../../utilities/mkcert.js' -import {generateCertificatePrompt} from '../../prompts/dev.js' -import {ports} from '../../constants.js' import {Flags} from '@oclif/core' import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' import {globalFlags} from '@shopify/cli-kit/node/cli' import {addPublicMetadata} from '@shopify/cli-kit/node/metadata' -import {renderInfo, renderWarning} from '@shopify/cli-kit/node/ui' -import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' export default class Dev extends AppCommand { static summary = 'Run the app.' @@ -147,69 +142,17 @@ If you're using the Ruby app template, then you need to complete the following s await showApiKeyDeprecationWarning() } - let tunnel: TunnelMode = {mode: 'auto'} - let tunnelType = 'cloudflare' - - if (flags['use-localhost']) { - tunnelType = 'use-localhost' - - // Check if a port was specified with --localhost-port - const requestedPort = flags['localhost-port'] ?? ports.localhost - const actualPort = (await checkPortAvailability(requestedPort)) - ? requestedPort - : await getAvailableTCPPort(ports.localhost) - - tunnel = { - mode: 'no-tunnel-use-localhost', - port: actualPort, - provideCertificate: async (appDirectory) => { - renderInfo({ - headline: 'Localhost-based development is in developer preview.', - body: [ - '`--use-localhost` is not compatible with Shopify features which directly invoke your app', - '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', - 'device (such as POS). Please report any issues and provide feedback on the dev community:', - ], - link: { - label: 'Create a feedback post', - url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', - }, - }) - - if (requestedPort && requestedPort !== actualPort) { - renderWarning({ - headline: [ - 'A random port will be used for localhost because', - {command: `${requestedPort}`}, - 'is not available.', - ], - body: [ - 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', - {command: '--localhost-port PORT'}, - 'flag.', - ], - }) - } - - return generateCertificate({ - appDirectory, - onRequiresConfirmation: generateCertificatePrompt, - }) - }, - } - } else if (flags['tunnel-url']) { - tunnelType = 'custom' - tunnel = { - mode: 'provided', - url: flags['tunnel-url'], - } - } + const tunnelMode = await getTunnelMode({ + useLocalhost: flags['use-localhost'], + tunnelUrl: flags['tunnel-url'], + localhostPort: flags['localhost-port'], + }) await addPublicMetadata(() => { return { cmd_app_dependency_installation_skipped: flags['skip-dependencies-installation'], cmd_app_reset_used: flags.reset, - cmd_dev_tunnel_type: tunnelType, + cmd_dev_tunnel_type: tunnelMode.mode, } }) @@ -241,7 +184,7 @@ If you're using the Ruby app template, then you need to complete the following s notify: flags.notify, graphiqlPort: flags['graphiql-port'], graphiqlKey: flags['graphiql-key'], - tunnel, + tunnel: tunnelMode, } await dev(devOptions) diff --git a/packages/app/src/cli/services/dev.test.ts b/packages/app/src/cli/services/dev.test.ts index 57ef4b625e..11f2a8e245 100644 --- a/packages/app/src/cli/services/dev.test.ts +++ b/packages/app/src/cli/services/dev.test.ts @@ -1,10 +1,22 @@ -import {developerPreviewController, warnIfScopesDifferBeforeDev} from './dev.js' +import { + AutoTunnel, + CustomTunnel, + developerPreviewController, + getTunnelMode, + NoTunnel, + warnIfScopesDifferBeforeDev, +} from './dev.js' import {fetchAppPreviewMode} from './dev/fetch.js' import {testAppLinked, testDeveloperPlatformClient, testOrganizationApp} from '../models/app/app.test-data.js' -import {describe, expect, test, vi} from 'vitest' +import {generateCertificate} from '../utilities/mkcert.js' +import {ports} from '../constants.js' +import {describe, expect, test, vi, beforeEach} from 'vitest' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' +import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' vi.mock('./dev/fetch.js') +vi.mock('@shopify/cli-kit/node/tcp') +vi.mock('../utilities/mkcert.js') describe('developerPreviewController', () => { test('does not refresh the tokens when they are still valid', async () => { @@ -97,3 +109,169 @@ describe('warnIfScopesDifferBeforeDev', () => { expect(mockOutput.warn()).toBe('') }) }) + +describe('getTunnelMode() if useLocalhost is true', () => { + const mockCertificate = { + keyContent: 'test-key-content', + certContent: 'test-cert-content', + certPath: '/path/to/cert', + } + + beforeEach(() => { + vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) + }) + + const defaultOptions = { + useLocalhost: true, + localhostPort: undefined, + tunnelUrl: undefined, + } + + test('returns localhostPort when passed', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode({ + ...defaultOptions, + localhostPort, + })) as NoTunnel + + // Then + expect(getAvailableTCPPort).toHaveBeenCalledWith(localhostPort) + expect(result).toMatchObject({ + mode: 'use-localhost', + port: localhostPort, + provideCertificate: expect.any(Function), + }) + }) + + test('returns ports.localhost when localhostPort is not passed', async () => { + // Given + vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost) + + // When + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + + // Then + expect(getAvailableTCPPort).toHaveBeenCalledWith(ports.localhost) + expect(result).toMatchObject({ + mode: 'use-localhost', + port: ports.localhost, + provideCertificate: expect.any(Function), + }) + }) + + describe('provideCertificate()', () => { + test('Calls renderInfo', async () => { + // Given + vi.mocked(checkPortAvailability).mockResolvedValue(true) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(mockOutput.info()).toContain('Localhost-based development is in developer preview') + }) + + test('Shows warning if requestedPort is not available', async () => { + // Given + const requestedPort = 3000 + const availablePort = 3001 + vi.mocked(checkPortAvailability).mockResolvedValue(false) + vi.mocked(getAvailableTCPPort).mockResolvedValue(availablePort) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(result.port).toBe(availablePort) + expect(mockOutput.warn()).toContain('A random port will be used for localhost') + }) + + test('Shows warning if requestedPort is not passed and ports.localhost is not available', async () => { + // Given + const availablePort = 3001 + vi.mocked(checkPortAvailability).mockResolvedValue(false) + vi.mocked(getAvailableTCPPort).mockResolvedValue(availablePort) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(result.port).toBe(availablePort) + expect(mockOutput.warn()).toContain('A random port will be used for localhost') + }) + + test('Calls generateCertificate and returns its value', async () => { + // Given + vi.mocked(checkPortAvailability).mockResolvedValue(true) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + const certificate = await result.provideCertificate('app-directory') + + // Then + expect(generateCertificate).toHaveBeenCalledWith({ + appDirectory: 'app-directory', + onRequiresConfirmation: expect.any(Function), + }) + expect(certificate).toEqual(mockCertificate) + }) + }) +}) + +describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: 'https://my-tunnel-url.com', + } + + test('returns CustomTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as CustomTunnel + + // Then + expect(result).toMatchObject({ + mode: 'custom', + url: defaultOptions.tunnelUrl, + }) + }) +}) + +describe('getTunnelMode() if useLocalhost is false and tunnelUrl is undefined', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: undefined, + } + + test('returns AutoTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as AutoTunnel + + // Then + expect(result).toMatchObject({mode: 'auto'}) + }) +}) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index c865860fcc..b4dca48208 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -33,6 +33,8 @@ import {ports} from '../constants.js' import metadata from '../metadata.js' import {AppConfigurationUsedByCli} from '../models/extensions/specifications/types/app_config.js' import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' +import {generateCertificate} from '../utilities/mkcert.js' +import {generateCertificatePrompt} from '../prompts/dev.js' import {Config} from '@oclif/core' import {performActionWithRetryAfterRecovery} from '@shopify/cli-kit/common/retry' import {AbortController} from '@shopify/cli-kit/node/abort' @@ -40,29 +42,28 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/ import {TunnelClient} from '@shopify/cli-kit/node/plugins/tunnel' import {getBackendPort} from '@shopify/cli-kit/node/environment' import {basename} from '@shopify/cli-kit/node/path' -import {renderWarning} from '@shopify/cli-kit/node/ui' +import {renderWarning, renderInfo} from '@shopify/cli-kit/node/ui' import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {OutputProcess, formatPackageManagerCommand, outputDebug} from '@shopify/cli-kit/node/output' import {hashString} from '@shopify/cli-kit/node/crypto' import {AbortError} from '@shopify/cli-kit/node/error' interface NoTunnel { - mode: 'no-tunnel-use-localhost' - port?: number + mode: 'use-localhost' + port: number provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> } -interface AutoTunnel { +export interface AutoTunnel { mode: 'auto' } -interface TunnelProvided { - mode: 'provided' +export interface CustomTunnel { + mode: 'custom' url: string } -export type TunnelMode = NoTunnel | AutoTunnel | TunnelProvided - +type TunnelMode = NoTunnel | AutoTunnel | CustomTunnel export interface DevOptions { app: AppLinkedInterface remoteApp: OrganizationApp @@ -324,10 +325,10 @@ async function setupNetworkingOptions( // we can rename them to proxyUrl and proxyPort when we delete dev.ts const [{frontendUrl, frontendPort: proxyPort, usingLocalhost}, backendPort, currentUrls] = await Promise.all([ generateFrontendURL({ - noTunnelUseLocalhost: tunnelOptions.mode === 'no-tunnel-use-localhost', - tunnelUrl: tunnelOptions.mode === 'provided' ? tunnelOptions.url : undefined, + noTunnelUseLocalhost: tunnelOptions.mode === 'use-localhost', + tunnelUrl: tunnelOptions.mode === 'custom' ? tunnelOptions.url : undefined, tunnelClient, - port: tunnelOptions.mode === 'no-tunnel-use-localhost' ? tunnelOptions.port : undefined, + port: tunnelOptions.mode === 'use-localhost' ? tunnelOptions.port : undefined, }), getBackendPort() ?? backendConfig?.configuration.port ?? getAvailableTCPPort(), getURLs(remoteAppConfig), @@ -344,7 +345,7 @@ async function setupNetworkingOptions( frontendPort = frontendPort ?? (await getAvailableTCPPort()) let reverseProxyCert - if (tunnelOptions.mode === 'no-tunnel-use-localhost') { + if (tunnelOptions.mode === 'use-localhost') { const {keyContent, certContent, certPath} = await tunnelOptions.provideCertificate(appDirectory) reverseProxyCert = { key: keyContent, @@ -527,3 +528,71 @@ async function validateCustomPorts(webConfigs: Web[], graphiqlPort: number) { function setPreviousAppId(directory: string, apiKey: string) { setCachedAppInfo({directory, previousAppId: apiKey}) } + +/** + * Gets the tunnel config for doing app dev + * @param options - Options required for the config + * @returns A tunnel configuration object + */ + +export async function getTunnelMode({ + useLocalhost, + localhostPort, + tunnelUrl, +}: { + useLocalhost?: boolean + localhostPort?: number + tunnelUrl?: string +}): Promise { + if (useLocalhost) { + const requestedPort = localhostPort ?? ports.localhost + const actualPort = await getAvailableTCPPort(requestedPort) + + return { + mode: 'use-localhost', + port: actualPort, + provideCertificate: async (appDirectory) => { + renderInfo({ + headline: 'Localhost-based development is in developer preview.', + body: [ + '`--use-localhost` is not compatible with Shopify features which directly invoke your app', + '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', + 'device (such as POS). Please report any issues and provide feedback on the dev community:', + ], + link: { + label: 'Create a feedback post', + url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', + }, + }) + + if (requestedPort !== actualPort) { + renderWarning({ + headline: [ + 'A random port will be used for localhost because', + {command: `${requestedPort}`}, + 'is not available.', + ], + body: [ + 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', + {command: '--localhost-port PORT'}, + 'flag.', + ], + }) + } + + return generateCertificate({ + appDirectory, + onRequiresConfirmation: generateCertificatePrompt, + }) + }, + } + } + + if (tunnelUrl) { + return {mode: 'custom', url: tunnelUrl} + } + + return { + mode: 'auto', + } +} diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts index fb550798a8..a5ab9c50a5 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.ts @@ -269,12 +269,10 @@ export const startProxyServer: DevProcessFunction<{ rules: {[key: string]: string} localhostCert?: LocalhostCert }> = async ({abortSignal, stdout}, {port, rules, localhostCert}) => { - const {server, port: certPort} = await getProxyingWebServer(rules, abortSignal, localhostCert) - // Use the port from the certificate if available, otherwise use the provided port - const actualPort = certPort ?? port + const {server} = await getProxyingWebServer(rules, abortSignal, localhostCert) outputInfo( - `Proxy server started on port ${actualPort} ${localhostCert ? `with certificate ${localhostCert.certPath}` : ''}`, + `Proxy server started on port ${port} ${localhostCert ? `with certificate ${localhostCert.certPath}` : ''}`, stdout, ) - await server.listen(actualPort) + await server.listen(port) } From 95631304a04d060b8df1b51d7e0464fcb594e95f Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Thu, 10 Apr 2025 09:33:45 -0400 Subject: [PATCH 03/12] Ran pnpm refresh-manifests --- packages/cli/oclif.manifest.json | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index a1ba85e80a..ec60101dc1 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -453,6 +453,18 @@ "name": "graphiql-port", "type": "option" }, + "localhost-port": { + "dependsOn": [ + "use-localhost" + ], + "description": "Port to use for localhost. Only applicable when --use-localhost is specified.", + "env": "SHOPIFY_FLAG_LOCALHOST_PORT", + "hasDynamicHelp": false, + "hidden": true, + "multiple": false, + "name": "localhost-port", + "type": "option" + }, "no-color": { "allowNo": false, "description": "Disable color output.", @@ -557,7 +569,7 @@ }, "use-localhost": { "allowNo": false, - "description": "Service entry point will listen to localhost. A tunnel won't be used. Will work for testing many app features, but not Webhooks, Flow Action, App Proxy or POS", + "description": "Service entry point will listen to localhost. A tunnel won't be used. Will work for testing many app features, but not those that directly invoke your app (E.g: Webhooks)", "env": "SHOPIFY_FLAG_USE_LOCALHOST", "exclusive": [ "tunnel-url" From 32291cf24eae8a65308d3c14c26220268ff2b362 Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Thu, 10 Apr 2025 10:16:41 -0400 Subject: [PATCH 04/12] Make types a little more explicit Previously the options passed to generateFrontendURL were an object with a bunch of optional properties. But if noTunnelUseLocalhost is true then port is mandatory and it's alwasy passed. By switching options to a union typescriot understands this and we no longer need to have get an available TCP port for localhost... it would alwasy be passed --- packages/app/src/cli/services/dev.ts | 22 ++++++++++++------ .../app/src/cli/services/dev/urls.test.ts | 4 ++-- packages/app/src/cli/services/dev/urls.ts | 23 ++++++++++++------- 3 files changed, 32 insertions(+), 17 deletions(-) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index b4dca48208..22e4225e05 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -1,5 +1,6 @@ import { ApplicationURLs, + FrontendURLOptions, generateApplicationURLs, generateFrontendURL, getURLs, @@ -48,7 +49,7 @@ import {OutputProcess, formatPackageManagerCommand, outputDebug} from '@shopify/ import {hashString} from '@shopify/cli-kit/node/crypto' import {AbortError} from '@shopify/cli-kit/node/error' -interface NoTunnel { +export interface NoTunnel { mode: 'use-localhost' port: number provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> @@ -321,15 +322,22 @@ async function setupNetworkingOptions( await validateCustomPorts(webs, graphiqlPort) + const frontendUrlOptions: FrontendURLOptions = + tunnelOptions.mode === 'use-localhost' + ? { + noTunnelUseLocalhost: true, + port: tunnelOptions.port, + } + : { + noTunnelUseLocalhost: false, + tunnelUrl: tunnelOptions.mode === 'custom' ? tunnelOptions.url : undefined, + tunnelClient, + } + // generateFrontendURL still uses the old naming of frontendUrl and frontendPort, // we can rename them to proxyUrl and proxyPort when we delete dev.ts const [{frontendUrl, frontendPort: proxyPort, usingLocalhost}, backendPort, currentUrls] = await Promise.all([ - generateFrontendURL({ - noTunnelUseLocalhost: tunnelOptions.mode === 'use-localhost', - tunnelUrl: tunnelOptions.mode === 'custom' ? tunnelOptions.url : undefined, - tunnelClient, - port: tunnelOptions.mode === 'use-localhost' ? tunnelOptions.port : undefined, - }), + generateFrontendURL(frontendUrlOptions), getBackendPort() ?? backendConfig?.configuration.port ?? getAvailableTCPPort(), getURLs(remoteAppConfig), ]) diff --git a/packages/app/src/cli/services/dev/urls.test.ts b/packages/app/src/cli/services/dev/urls.test.ts index fb7505ffd2..573d40aa9f 100644 --- a/packages/app/src/cli/services/dev/urls.test.ts +++ b/packages/app/src/cli/services/dev/urls.test.ts @@ -450,13 +450,13 @@ describe('generateFrontendURL', () => { test('returns localhost if noTunnelUseLocalhost is true', async () => { // Given - const options = {...defaultOptions, noTunnelUseLocalhost: true} + const options: FrontendURLOptions = {noTunnelUseLocalhost: true, port: 1234} // When const got = await generateFrontendURL(options) // Then - expect(got).toEqual({frontendUrl: 'https://localhost', frontendPort: 3042, usingLocalhost: true}) + expect(got).toEqual({frontendUrl: 'https://localhost', frontendPort: 1234, usingLocalhost: true}) expect(renderSelectPrompt).not.toBeCalled() }) diff --git a/packages/app/src/cli/services/dev/urls.ts b/packages/app/src/cli/services/dev/urls.ts index 65ffed4d2f..72571c440b 100644 --- a/packages/app/src/cli/services/dev/urls.ts +++ b/packages/app/src/cli/services/dev/urls.ts @@ -7,7 +7,7 @@ import {DeveloperPlatformClient} from '../../utilities/developer-platform-client import {patchAppConfigurationFile} from '../app/patch-app-configuration-file.js' import {AbortError, BugError} from '@shopify/cli-kit/node/error' import {Config} from '@oclif/core' -import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' +import {checkPortAvailability} from '@shopify/cli-kit/node/tcp' import {isValidURL} from '@shopify/cli-kit/common/url' import {appHost, appPort, fetchSpinPort, isSpin, spinFqdn, spinVariables} from '@shopify/cli-kit/node/context/spin' import {codespaceURL, codespacePortForwardingDomain, gitpodURL} from '@shopify/cli-kit/node/context/local' @@ -28,11 +28,19 @@ export interface ApplicationURLs { appProxy?: AppProxy } -export interface FrontendURLOptions { - noTunnelUseLocalhost: boolean +export type FrontendURLOptions = UseLocalhostFrontendUrlOptions | UseTunnelFrontendUrlOptions + +interface UseLocalhostFrontendUrlOptions { + noTunnelUseLocalhost: true + port: number + tunnelUrl?: undefined + tunnelClient?: undefined +} + +interface UseTunnelFrontendUrlOptions { + noTunnelUseLocalhost: false tunnelUrl?: string tunnelClient: TunnelClient | undefined - port?: number } interface FrontendURLResult { @@ -43,7 +51,7 @@ interface FrontendURLResult { /** * The tunnel creation logic depends on 7 variables: - * - If a Codespaces environment is deteced, then the URL is built using the codespaces hostname. No need for tunnel + * - If a Codespaces environment is detected, then the URL is built using the codespaces hostname. No need for tunnel * - If a Gitpod environment is detected, then the URL is built using the gitpod hostname. No need for tunnel * - If a Spin environment is detected, then the URL is built using the cli + fqdn hostname as configured in nginx. * No need for tunnel. In case problems with that configuration, the flags Tunnel or Custom Tunnel url could be used @@ -100,12 +108,11 @@ export async function generateFrontendURL(options: FrontendURLOptions): Promise< } if (options.noTunnelUseLocalhost) { - frontendPort = options.port ?? (await getAvailableTCPPort()) + frontendPort = options.port frontendUrl = 'https://localhost' } else if (options.tunnelClient) { - const url = await pollTunnelURL(options.tunnelClient) frontendPort = options.tunnelClient.port - frontendUrl = url + frontendUrl = await pollTunnelURL(options.tunnelClient) } return {frontendUrl, frontendPort, usingLocalhost} From 17590fc055d831636a30e3e80936d422ee005f86 Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Fri, 11 Apr 2025 11:01:08 -0400 Subject: [PATCH 05/12] Respond to PR Feedback 1. --localhost-port no longer requires the --use-localhost flag 2. If --localhost-port is specified, but the port is not free, the process exist with a warning message 3. Improve the argument typing for generateFrontendURL() --- packages/app/src/cli/commands/app/dev.ts | 3 +- packages/app/src/cli/constants.ts | 2 +- packages/app/src/cli/services/dev.test.ts | 124 +++++++++++----------- packages/app/src/cli/services/dev.ts | 101 ++++++++++-------- packages/app/src/cli/services/dev/urls.ts | 15 +-- packages/cli/oclif.manifest.json | 5 +- 6 files changed, 127 insertions(+), 123 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index f4264c3d15..1c8b18a47d 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -95,9 +95,8 @@ If you're using the Ruby app template, then you need to complete the following s }), 'localhost-port': Flags.integer({ hidden: true, - description: 'Port to use for localhost. Only applicable when --use-localhost is specified.', + description: 'Port to use for localhost.', env: 'SHOPIFY_FLAG_LOCALHOST_PORT', - dependsOn: ['use-localhost'], }), theme: Flags.string({ hidden: false, diff --git a/packages/app/src/cli/constants.ts b/packages/app/src/cli/constants.ts index 6922035558..150690cf1b 100644 --- a/packages/app/src/cli/constants.ts +++ b/packages/app/src/cli/constants.ts @@ -37,7 +37,7 @@ export const blocks = { export const ports = { graphiql: 3457, - localhost: 3568, + localhost: 3458, } as const export const EsbuildEnvVarRegex = /^([a-zA-Z_$])([a-zA-Z0-9_$])*$/ diff --git a/packages/app/src/cli/services/dev.test.ts b/packages/app/src/cli/services/dev.test.ts index 11f2a8e245..0045ab969d 100644 --- a/packages/app/src/cli/services/dev.test.ts +++ b/packages/app/src/cli/services/dev.test.ts @@ -110,6 +110,49 @@ describe('warnIfScopesDifferBeforeDev', () => { }) }) +describe('getTunnelMode() if tunnelUrl is defined', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: undefined, + } + + test('returns AutoTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as AutoTunnel + + // Then + expect(result).toMatchObject({mode: 'auto'}) + }) +}) + +describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: 'https://my-tunnel-url.com', + } + + test('returns CustomTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as CustomTunnel + + // Then + expect(result).toMatchObject({ + mode: 'custom', + url: defaultOptions.tunnelUrl, + }) + }) +}) + describe('getTunnelMode() if useLocalhost is true', () => { const mockCertificate = { keyContent: 'test-key-content', @@ -147,6 +190,20 @@ describe('getTunnelMode() if useLocalhost is true', () => { }) }) + test('throws when localhostPort is passed, but not available', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(4321) + + // Then + await expect( + getTunnelMode({ + ...defaultOptions, + localhostPort, + }), + ).rejects.toThrow() + }) + test('returns ports.localhost when localhostPort is not passed', async () => { // Given vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost) @@ -178,29 +235,11 @@ describe('getTunnelMode() if useLocalhost is true', () => { expect(mockOutput.info()).toContain('Localhost-based development is in developer preview') }) - test('Shows warning if requestedPort is not available', async () => { - // Given - const requestedPort = 3000 - const availablePort = 3001 - vi.mocked(checkPortAvailability).mockResolvedValue(false) - vi.mocked(getAvailableTCPPort).mockResolvedValue(availablePort) - - // When - const mockOutput = mockAndCaptureOutput() - mockOutput.clear() - const result = (await getTunnelMode(defaultOptions)) as NoTunnel - await result.provideCertificate('app-directory') - - // Then - expect(result.port).toBe(availablePort) - expect(mockOutput.warn()).toContain('A random port will be used for localhost') - }) - - test('Shows warning if requestedPort is not passed and ports.localhost is not available', async () => { + test('Renders warning if ports.localhost is not available', async () => { // Given - const availablePort = 3001 + const availablePort = ports.localhost + 1 vi.mocked(checkPortAvailability).mockResolvedValue(false) - vi.mocked(getAvailableTCPPort).mockResolvedValue(availablePort) + vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) // When const mockOutput = mockAndCaptureOutput() @@ -232,46 +271,3 @@ describe('getTunnelMode() if useLocalhost is true', () => { }) }) }) - -describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: 'https://my-tunnel-url.com', - } - - test('returns CustomTunnel', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) - - // When - const result = (await getTunnelMode(defaultOptions)) as CustomTunnel - - // Then - expect(result).toMatchObject({ - mode: 'custom', - url: defaultOptions.tunnelUrl, - }) - }) -}) - -describe('getTunnelMode() if useLocalhost is false and tunnelUrl is undefined', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: undefined, - } - - test('returns AutoTunnel', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) - - // When - const result = (await getTunnelMode(defaultOptions)) as AutoTunnel - - // Then - expect(result).toMatchObject({mode: 'auto'}) - }) -}) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 22e4225e05..3ea9b9b28b 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -526,7 +526,7 @@ async function validateCustomPorts(webConfigs: Web[], graphiqlPort: number) { const portAvailable = await checkPortAvailability(graphiqlPort) if (!portAvailable) { const errorMessage = `Port ${graphiqlPort} is not available for serving GraphiQL.` - const tryMessage = ['Choose a different port by setting the', {command: '--graphiql-port'}, 'flag.'] + const tryMessage = ['Choose a different port for the', {command: '--graphiql-port'}, 'flag.'] throw new AbortError(errorMessage, tryMessage) } })(), @@ -538,69 +538,78 @@ function setPreviousAppId(directory: string, apiKey: string) { } /** - * Gets the tunnel config for doing app dev + * Gets the tunnel or localhost config for doing app dev * @param options - Options required for the config * @returns A tunnel configuration object */ - export async function getTunnelMode({ useLocalhost, localhostPort, tunnelUrl, }: { + tunnelUrl?: string useLocalhost?: boolean localhostPort?: number - tunnelUrl?: string }): Promise { - if (useLocalhost) { - const requestedPort = localhostPort ?? ports.localhost - const actualPort = await getAvailableTCPPort(requestedPort) + // Developer brought their own tunnel + if (tunnelUrl) { + return {mode: 'custom', url: tunnelUrl} + } + // CLI should create a tunnel + if (!useLocalhost && !localhostPort) { return { - mode: 'use-localhost', - port: actualPort, - provideCertificate: async (appDirectory) => { - renderInfo({ - headline: 'Localhost-based development is in developer preview.', - body: [ - '`--use-localhost` is not compatible with Shopify features which directly invoke your app', - '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', - 'device (such as POS). Please report any issues and provide feedback on the dev community:', - ], - link: { - label: 'Create a feedback post', - url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', - }, - }) - - if (requestedPort !== actualPort) { - renderWarning({ - headline: [ - 'A random port will be used for localhost because', - {command: `${requestedPort}`}, - 'is not available.', - ], - body: [ - 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', - {command: '--localhost-port PORT'}, - 'flag.', - ], - }) - } - - return generateCertificate({ - appDirectory, - onRequiresConfirmation: generateCertificatePrompt, - }) - }, + mode: 'auto', } } - if (tunnelUrl) { - return {mode: 'custom', url: tunnelUrl} + const requestedPort = localhostPort ?? ports.localhost + const actualPort = await getAvailableTCPPort(requestedPort) + + // The user specified a port. It's not available. Abort! + if (localhostPort && actualPort !== requestedPort) { + const errorMessage = `Port ${localhostPort} is not available.` + const tryMessage = ['Choose a different port for the', {command: '--localhost-port'}, 'flag.'] + throw new AbortError(errorMessage, tryMessage) } return { - mode: 'auto', + mode: 'use-localhost', + port: actualPort, + provideCertificate: async (appDirectory) => { + renderInfo({ + headline: 'Localhost-based development is in developer preview.', + body: [ + '`--use-localhost` is not compatible with Shopify features which directly invoke your app', + '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', + 'device (such as POS). Please report any issues and provide feedback on the dev community:', + ], + link: { + label: 'Create a feedback post', + url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', + }, + }) + + // The user didn't specify a port. The default isn't available. Warn + if (requestedPort !== actualPort) { + renderWarning({ + headline: [ + 'A random port will be used for localhost because', + {command: `${requestedPort}`}, + 'is not available.', + ], + body: [ + 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', + {command: '--localhost-port PORT'}, + 'flag.', + ], + }) + } + + return generateCertificate({ + appDirectory, + onRequiresConfirmation: generateCertificatePrompt, + }) + }, } } diff --git a/packages/app/src/cli/services/dev/urls.ts b/packages/app/src/cli/services/dev/urls.ts index 72571c440b..097ce1a7ff 100644 --- a/packages/app/src/cli/services/dev/urls.ts +++ b/packages/app/src/cli/services/dev/urls.ts @@ -33,8 +33,6 @@ export type FrontendURLOptions = UseLocalhostFrontendUrlOptions | UseTunnelFront interface UseLocalhostFrontendUrlOptions { noTunnelUseLocalhost: true port: number - tunnelUrl?: undefined - tunnelClient?: undefined } interface UseTunnelFrontendUrlOptions { @@ -62,6 +60,14 @@ interface FrontendURLResult { * If there is no cached tunnel plugin and a tunnel is necessary, we'll ask the user to confirm. */ export async function generateFrontendURL(options: FrontendURLOptions): Promise { + if (options.noTunnelUseLocalhost) { + return { + frontendUrl: 'https://localhost', + frontendPort: options.port, + usingLocalhost: true, + } + } + let frontendPort = 4040 let frontendUrl = '' const usingLocalhost = options.noTunnelUseLocalhost @@ -107,10 +113,7 @@ export async function generateFrontendURL(options: FrontendURLOptions): Promise< return {frontendUrl, frontendPort, usingLocalhost} } - if (options.noTunnelUseLocalhost) { - frontendPort = options.port - frontendUrl = 'https://localhost' - } else if (options.tunnelClient) { + if (options.tunnelClient) { frontendPort = options.tunnelClient.port frontendUrl = await pollTunnelURL(options.tunnelClient) } diff --git a/packages/cli/oclif.manifest.json b/packages/cli/oclif.manifest.json index ec60101dc1..ee48f6596c 100644 --- a/packages/cli/oclif.manifest.json +++ b/packages/cli/oclif.manifest.json @@ -454,10 +454,7 @@ "type": "option" }, "localhost-port": { - "dependsOn": [ - "use-localhost" - ], - "description": "Port to use for localhost. Only applicable when --use-localhost is specified.", + "description": "Port to use for localhost.", "env": "SHOPIFY_FLAG_LOCALHOST_PORT", "hasDynamicHelp": false, "hidden": true, From 7cdc12944582ef988299f60630b503a9c1b57acc Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Fri, 11 Apr 2025 13:32:27 -0400 Subject: [PATCH 06/12] Added a changeset --- .changeset/clever-dogs-rush.md | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 .changeset/clever-dogs-rush.md diff --git a/.changeset/clever-dogs-rush.md b/.changeset/clever-dogs-rush.md new file mode 100644 index 0000000000..0d03ebae19 --- /dev/null +++ b/.changeset/clever-dogs-rush.md @@ -0,0 +1,10 @@ +--- +'@shopify/app': minor +--- + +Improved how port selection when using localhost development + +Added a `--localhost-port` flag. Use this to specify that you want to develop using localhost on a specific port. For example: `shopify app dev --localhost-port=4000` + +`shopify app dev --use-localhost` will always try to use port 3458. If port 3458 is not available the CLI will warn the user and select a different port. + From 31eb92a869edebeb66390530d3002cf5e6125aec Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Mon, 21 Apr 2025 09:07:00 -0400 Subject: [PATCH 07/12] PR Feedback: * Fix typo in changelog * If graphiql and localhost port is already in use, we will only show one renderWarning message. We do this by combining 2 messages into 1 --- .changeset/clever-dogs-rush.md | 2 +- packages/app/src/cli/commands/app/dev.ts | 8 +- packages/app/src/cli/services/dev.test.ts | 107 ++++++++++++--- packages/app/src/cli/services/dev.ts | 129 +++++++++++++----- .../dev/processes/setup-dev-processes.test.ts | 5 + 5 files changed, 200 insertions(+), 51 deletions(-) diff --git a/.changeset/clever-dogs-rush.md b/.changeset/clever-dogs-rush.md index 0d03ebae19..05941d33a9 100644 --- a/.changeset/clever-dogs-rush.md +++ b/.changeset/clever-dogs-rush.md @@ -2,7 +2,7 @@ '@shopify/app': minor --- -Improved how port selection when using localhost development +Improved how port selection works when using localhost development Added a `--localhost-port` flag. Use this to specify that you want to develop using localhost on a specific port. For example: `shopify app dev --localhost-port=4000` diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 1c8b18a47d..366af24bed 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -1,5 +1,5 @@ import {appFlags} from '../../flags.js' -import {dev, DevOptions, getTunnelMode} from '../../services/dev.js' +import {dev, DevOptions, getTunnelMode, PortWarning} from '../../services/dev.js' import {showApiKeyDeprecationWarning} from '../../prompts/deprecation-warnings.js' import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppCommand, {AppCommandOutput} from '../../utilities/app-command.js' @@ -141,10 +141,15 @@ If you're using the Ruby app template, then you need to complete the following s await showApiKeyDeprecationWarning() } + // getTunnelMode can populate this array. This array is then passed to the dev function. + // This allows the dev function to group port warnings into a single renderWarning + const portWarnings: PortWarning[] = [] + const tunnelMode = await getTunnelMode({ useLocalhost: flags['use-localhost'], tunnelUrl: flags['tunnel-url'], localhostPort: flags['localhost-port'], + portWarnings, }) await addPublicMetadata(() => { @@ -184,6 +189,7 @@ If you're using the Ruby app template, then you need to complete the following s graphiqlPort: flags['graphiql-port'], graphiqlKey: flags['graphiql-key'], tunnel: tunnelMode, + portWarnings, } await dev(devOptions) diff --git a/packages/app/src/cli/services/dev.test.ts b/packages/app/src/cli/services/dev.test.ts index 0045ab969d..923f709e16 100644 --- a/packages/app/src/cli/services/dev.test.ts +++ b/packages/app/src/cli/services/dev.test.ts @@ -4,6 +4,8 @@ import { developerPreviewController, getTunnelMode, NoTunnel, + PortWarning, + renderPortWarnings, warnIfScopesDifferBeforeDev, } from './dev.js' import {fetchAppPreviewMode} from './dev/fetch.js' @@ -110,11 +112,75 @@ describe('warnIfScopesDifferBeforeDev', () => { }) }) +describe('renderPortWarnings()', () => { + test('does not call renderWarning when no warnings', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toBe('') + }) + + test('calls renderWarning when there is a warning', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: 1234, + }, + ] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toContain('A random port will be used for localhost because 1234 is not available.') + expect(mockOutput.warn()).toContain('If you want to use a specific port, you can choose a different one by') + expect(mockOutput.warn()).toContain('setting the `--localhost-port` flag.') + }) + + test('Combines warnings when there are multiple', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: 1234, + }, + { + type: 'GraphiQL', + flag: '--graphiql-port', + requestedPort: 5678, + }, + ] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toContain('Random ports will be used for localhost and GraphiQL because the requested') + expect(mockOutput.warn()).toContain('ports are not available') + expect(mockOutput.warn()).toContain('If you want to use specific ports, you can choose different ports using') + expect(mockOutput.warn()).toContain('the `--localhost-port` and `--graphiql-port` flags.') + }) +}) + describe('getTunnelMode() if tunnelUrl is defined', () => { const defaultOptions = { useLocalhost: false, localhostPort: undefined, tunnelUrl: undefined, + portWarnings: [], } test('returns AutoTunnel', async () => { @@ -135,6 +201,7 @@ describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', ( useLocalhost: false, localhostPort: undefined, tunnelUrl: 'https://my-tunnel-url.com', + portWarnings: [], } test('returns CustomTunnel', async () => { @@ -168,6 +235,7 @@ describe('getTunnelMode() if useLocalhost is true', () => { useLocalhost: true, localhostPort: undefined, tunnelUrl: undefined, + portWarnings: [], } test('returns localhostPort when passed', async () => { @@ -220,6 +288,28 @@ describe('getTunnelMode() if useLocalhost is true', () => { }) }) + test('Warns if ports.localhost is not available', async () => { + // Given + const availablePort = ports.localhost + 1 + vi.mocked(checkPortAvailability).mockResolvedValue(false) + vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) + const portWarnings: PortWarning[] = [] + + // When + const result = (await getTunnelMode({...defaultOptions, portWarnings})) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(result.port).toBe(availablePort) + expect(portWarnings).toStrictEqual([ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: ports.localhost, + }, + ]) + }) + describe('provideCertificate()', () => { test('Calls renderInfo', async () => { // Given @@ -235,23 +325,6 @@ describe('getTunnelMode() if useLocalhost is true', () => { expect(mockOutput.info()).toContain('Localhost-based development is in developer preview') }) - test('Renders warning if ports.localhost is not available', async () => { - // Given - const availablePort = ports.localhost + 1 - vi.mocked(checkPortAvailability).mockResolvedValue(false) - vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) - - // When - const mockOutput = mockAndCaptureOutput() - mockOutput.clear() - const result = (await getTunnelMode(defaultOptions)) as NoTunnel - await result.provideCertificate('app-directory') - - // Then - expect(result.port).toBe(availablePort) - expect(mockOutput.warn()).toContain('A random port will be used for localhost') - }) - test('Calls generateCertificate and returns its value', async () => { // Given vi.mocked(checkPortAvailability).mockResolvedValue(true) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 3ea9b9b28b..edeaa239ff 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -43,7 +43,7 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/ import {TunnelClient} from '@shopify/cli-kit/node/plugins/tunnel' import {getBackendPort} from '@shopify/cli-kit/node/environment' import {basename} from '@shopify/cli-kit/node/path' -import {renderWarning, renderInfo} from '@shopify/cli-kit/node/ui' +import {renderWarning, renderInfo, Token} from '@shopify/cli-kit/node/ui' import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {OutputProcess, formatPackageManagerCommand, outputDebug} from '@shopify/cli-kit/node/output' import {hashString} from '@shopify/cli-kit/node/crypto' @@ -65,6 +65,19 @@ export interface CustomTunnel { } type TunnelMode = NoTunnel | AutoTunnel | CustomTunnel +export type PortWarning = ( + | { + type: 'GraphiQL' + flag: '--graphiql-port' + } + | { + type: 'localhost' + flag: '--localhost-port' + } +) & { + requestedPort: number +} + export interface DevOptions { app: AppLinkedInterface remoteApp: OrganizationApp @@ -84,6 +97,7 @@ export interface DevOptions { notify?: string graphiqlPort?: number graphiqlKey?: string + portWarnings: PortWarning[] } export async function dev(commandOptions: DevOptions) { @@ -95,7 +109,7 @@ export async function dev(commandOptions: DevOptions) { } async function prepareForDev(commandOptions: DevOptions): Promise { - const {app, remoteApp, developerPlatformClient, store, specifications} = commandOptions + const {app, remoteApp, developerPlatformClient, store, specifications, portWarnings} = commandOptions // Be optimistic about tunnel creation and do it as early as possible const tunnelPort = await getAvailableTCPPort() @@ -136,23 +150,18 @@ async function prepareForDev(commandOptions: DevOptions): Promise { } const graphiqlPort = commandOptions.graphiqlPort ?? (await getAvailableTCPPort(ports.graphiql)) - const {graphiqlKey} = commandOptions + const requestedGraphiqlPort = commandOptions.graphiqlPort ?? ports.graphiql - if (graphiqlPort !== (commandOptions.graphiqlPort ?? ports.graphiql)) { - renderWarning({ - headline: [ - 'A random port will be used for GraphiQL because', - {command: `${ports.graphiql}`}, - 'is not available.', - ], - body: [ - 'If you want to keep your session in GraphiQL, you can choose a different one by setting the', - {command: '--graphiql-port'}, - 'flag.', - ], + if (graphiqlPort !== requestedGraphiqlPort) { + portWarnings.push({ + type: 'GraphiQL', + requestedPort: requestedGraphiqlPort, + flag: '--graphiql-port', }) } + renderPortWarnings(portWarnings) + const {webs, ...network} = await setupNetworkingOptions( app.directory, app.webs, @@ -189,7 +198,7 @@ async function prepareForDev(commandOptions: DevOptions): Promise { network, partnerUrlsUpdated, graphiqlPort, - graphiqlKey, + graphiqlKey: commandOptions.graphiqlKey, } } @@ -546,10 +555,12 @@ export async function getTunnelMode({ useLocalhost, localhostPort, tunnelUrl, + portWarnings, }: { tunnelUrl?: string useLocalhost?: boolean localhostPort?: number + portWarnings: PortWarning[] }): Promise { // Developer brought their own tunnel if (tunnelUrl) { @@ -573,6 +584,17 @@ export async function getTunnelMode({ throw new AbortError(errorMessage, tryMessage) } + // The user didn't specify a port. The default isn't available. Add to warnings array + // This will be rendered using renderWarning later when dev() is called + // This allows us to consolidate all port warnings into one renderWarning message + if (requestedPort !== actualPort) { + portWarnings.push({ + type: 'localhost', + requestedPort, + flag: '--localhost-port', + }) + } + return { mode: 'use-localhost', port: actualPort, @@ -590,22 +612,6 @@ export async function getTunnelMode({ }, }) - // The user didn't specify a port. The default isn't available. Warn - if (requestedPort !== actualPort) { - renderWarning({ - headline: [ - 'A random port will be used for localhost because', - {command: `${requestedPort}`}, - 'is not available.', - ], - body: [ - 'If you want to use a specific port, choose a different one or free up the one you requested. Then re-run the command with the', - {command: '--localhost-port PORT'}, - 'flag.', - ], - }) - } - return generateCertificate({ appDirectory, onRequiresConfirmation: generateCertificatePrompt, @@ -613,3 +619,62 @@ export async function getTunnelMode({ }, } } + +export function renderPortWarnings(portWarnings: PortWarning[] = []) { + if (portWarnings.length === 0 || !portWarnings[0]) return + + if (portWarnings.length === 1) { + const warning = portWarnings[0] + + renderWarning({ + headline: [`A random port will be used for ${warning.type} because ${warning?.requestedPort} is not available.`], + body: [ + `If you want to use a specific port, you can choose a different one by setting the `, + {command: warning?.flag}, + ` flag.`, + ], + }) + return + } + + const formattedWarningTypes = asHumanFriendlyTokenList(portWarnings.map((warning) => warning.type)).join(' ') + const formattedFlags = asHumanFriendlyTokenList(portWarnings.map((warning) => ({command: warning.flag}))) + + renderWarning({ + headline: [`Random ports will be used for ${formattedWarningTypes} because the requested ports are not available.`], + body: [`If you want to use specific ports, you can choose different ports using the`, ...formattedFlags, `flags.`], + }) +} + +/** + * Converts an array of Tokens into a human friendly list + * + * Returns a new array that contains the items separated by commas, + * except for the last item, which is seperated by "and". + * This is useful for creating human-friendly sentences. + * + * @example + * ```ts + * const items = ['apple', 'banana', 'cherry']; + * const result = asHumanFriendlyList(items) + * + * //['apple', ',', 'banana', ',', 'and', 'cherry'] + * console.log(result); + * ``` + */ + +function asHumanFriendlyTokenList(items: Token[]): Token[] { + if (items.length < 2) { + return items + } + + return items.reduce((acc, item, index) => { + if (index === items.length - 1) { + acc.push('and') + } else if (index !== 0) { + acc.push(', ') + } + acc.push(item) + return acc + }, []) +} diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index eec9cb080c..5c52d00eef 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -95,6 +95,7 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, + portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -308,6 +309,7 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, + portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -380,6 +382,7 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, + portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -474,6 +477,7 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, + portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -558,6 +562,7 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, + portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', From 22d320f11cb8ee901617e817ab761ac5c2fe740e Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Mon, 21 Apr 2025 14:54:57 -0400 Subject: [PATCH 08/12] Ran pnpm graphql-codegen --- .../business-platform-organizations/generated/types.d.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index 4e5d9ce544..3c676bf56e 100644 --- a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -19,8 +19,6 @@ export type Scalars = { AccessRoleRecordId: {input: any; output: any} /** The ID for a ActionAudit. */ ActionAuditID: {input: any; output: any} - /** The ID for a Address. */ - AddressID: {input: any; output: any} /** The ID for a BusinessUser. */ BusinessUserID: {input: any; output: any} /** A signed decimal number, which supports arbitrary precision and is serialized as a string. */ From d719c8987ad720e125816a17d881e72847ba4666 Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Tue, 22 Apr 2025 09:28:40 -0400 Subject: [PATCH 09/12] PR Feedback: * Move asHumanfriendlyTokenList to cli-kit/common/array, rename to asHumanFriendlyArray, update asHumanFriendlyArray to accept a generic so the return array can be explicitly typed if needed * Move getTunnelMode() into its own file * Move renderPortWarnings() into its own file --- packages/app/src/cli/commands/app/dev.ts | 4 +- packages/app/src/cli/services/dev.test.ts | 249 +----------------- packages/app/src/cli/services/dev.ts | 168 +----------- .../cli/services/dev/port-warnings.test.ts | 66 +++++ .../app/src/cli/services/dev/port-warnings.ts | 41 +++ .../src/cli/services/dev/tunnel-mode.test.ts | 177 +++++++++++++ .../app/src/cli/services/dev/tunnel-mode.ts | 98 +++++++ packages/cli-kit/src/public/common/array.ts | 34 +++ 8 files changed, 424 insertions(+), 413 deletions(-) create mode 100644 packages/app/src/cli/services/dev/port-warnings.test.ts create mode 100644 packages/app/src/cli/services/dev/port-warnings.ts create mode 100644 packages/app/src/cli/services/dev/tunnel-mode.test.ts create mode 100644 packages/app/src/cli/services/dev/tunnel-mode.ts diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 366af24bed..0a9d879401 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -1,10 +1,12 @@ import {appFlags} from '../../flags.js' -import {dev, DevOptions, getTunnelMode, PortWarning} from '../../services/dev.js' +import {dev, DevOptions} from '../../services/dev.js' import {showApiKeyDeprecationWarning} from '../../prompts/deprecation-warnings.js' import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppCommand, {AppCommandOutput} from '../../utilities/app-command.js' import {linkedAppContext} from '../../services/app-context.js' import {storeContext} from '../../services/store-context.js' +import {PortWarning} from '../../services/dev/port-warnings.js' +import {getTunnelMode} from '../../services/dev/tunnel-mode.js' import {Flags} from '@oclif/core' import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' import {globalFlags} from '@shopify/cli-kit/node/cli' diff --git a/packages/app/src/cli/services/dev.test.ts b/packages/app/src/cli/services/dev.test.ts index 923f709e16..5d50d63c64 100644 --- a/packages/app/src/cli/services/dev.test.ts +++ b/packages/app/src/cli/services/dev.test.ts @@ -1,20 +1,8 @@ -import { - AutoTunnel, - CustomTunnel, - developerPreviewController, - getTunnelMode, - NoTunnel, - PortWarning, - renderPortWarnings, - warnIfScopesDifferBeforeDev, -} from './dev.js' +import {developerPreviewController, warnIfScopesDifferBeforeDev} from './dev.js' import {fetchAppPreviewMode} from './dev/fetch.js' import {testAppLinked, testDeveloperPlatformClient, testOrganizationApp} from '../models/app/app.test-data.js' -import {generateCertificate} from '../utilities/mkcert.js' -import {ports} from '../constants.js' -import {describe, expect, test, vi, beforeEach} from 'vitest' +import {describe, expect, test, vi} from 'vitest' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' -import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' vi.mock('./dev/fetch.js') vi.mock('@shopify/cli-kit/node/tcp') @@ -111,236 +99,3 @@ describe('warnIfScopesDifferBeforeDev', () => { expect(mockOutput.warn()).toBe('') }) }) - -describe('renderPortWarnings()', () => { - test('does not call renderWarning when no warnings', () => { - // Given - const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [] - - // When - mockOutput.clear() - renderPortWarnings(portWarnings) - - // Then - expect(mockOutput.warn()).toBe('') - }) - - test('calls renderWarning when there is a warning', () => { - // Given - const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [ - { - type: 'localhost', - flag: '--localhost-port', - requestedPort: 1234, - }, - ] - - // When - mockOutput.clear() - renderPortWarnings(portWarnings) - - // Then - expect(mockOutput.warn()).toContain('A random port will be used for localhost because 1234 is not available.') - expect(mockOutput.warn()).toContain('If you want to use a specific port, you can choose a different one by') - expect(mockOutput.warn()).toContain('setting the `--localhost-port` flag.') - }) - - test('Combines warnings when there are multiple', () => { - // Given - const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [ - { - type: 'localhost', - flag: '--localhost-port', - requestedPort: 1234, - }, - { - type: 'GraphiQL', - flag: '--graphiql-port', - requestedPort: 5678, - }, - ] - - // When - mockOutput.clear() - renderPortWarnings(portWarnings) - - // Then - expect(mockOutput.warn()).toContain('Random ports will be used for localhost and GraphiQL because the requested') - expect(mockOutput.warn()).toContain('ports are not available') - expect(mockOutput.warn()).toContain('If you want to use specific ports, you can choose different ports using') - expect(mockOutput.warn()).toContain('the `--localhost-port` and `--graphiql-port` flags.') - }) -}) - -describe('getTunnelMode() if tunnelUrl is defined', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: undefined, - portWarnings: [], - } - - test('returns AutoTunnel', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) - - // When - const result = (await getTunnelMode(defaultOptions)) as AutoTunnel - - // Then - expect(result).toMatchObject({mode: 'auto'}) - }) -}) - -describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: 'https://my-tunnel-url.com', - portWarnings: [], - } - - test('returns CustomTunnel', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) - - // When - const result = (await getTunnelMode(defaultOptions)) as CustomTunnel - - // Then - expect(result).toMatchObject({ - mode: 'custom', - url: defaultOptions.tunnelUrl, - }) - }) -}) - -describe('getTunnelMode() if useLocalhost is true', () => { - const mockCertificate = { - keyContent: 'test-key-content', - certContent: 'test-cert-content', - certPath: '/path/to/cert', - } - - beforeEach(() => { - vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) - }) - - const defaultOptions = { - useLocalhost: true, - localhostPort: undefined, - tunnelUrl: undefined, - portWarnings: [], - } - - test('returns localhostPort when passed', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) - - // When - const result = (await getTunnelMode({ - ...defaultOptions, - localhostPort, - })) as NoTunnel - - // Then - expect(getAvailableTCPPort).toHaveBeenCalledWith(localhostPort) - expect(result).toMatchObject({ - mode: 'use-localhost', - port: localhostPort, - provideCertificate: expect.any(Function), - }) - }) - - test('throws when localhostPort is passed, but not available', async () => { - // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(4321) - - // Then - await expect( - getTunnelMode({ - ...defaultOptions, - localhostPort, - }), - ).rejects.toThrow() - }) - - test('returns ports.localhost when localhostPort is not passed', async () => { - // Given - vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost) - - // When - const result = (await getTunnelMode(defaultOptions)) as NoTunnel - - // Then - expect(getAvailableTCPPort).toHaveBeenCalledWith(ports.localhost) - expect(result).toMatchObject({ - mode: 'use-localhost', - port: ports.localhost, - provideCertificate: expect.any(Function), - }) - }) - - test('Warns if ports.localhost is not available', async () => { - // Given - const availablePort = ports.localhost + 1 - vi.mocked(checkPortAvailability).mockResolvedValue(false) - vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) - const portWarnings: PortWarning[] = [] - - // When - const result = (await getTunnelMode({...defaultOptions, portWarnings})) as NoTunnel - await result.provideCertificate('app-directory') - - // Then - expect(result.port).toBe(availablePort) - expect(portWarnings).toStrictEqual([ - { - type: 'localhost', - flag: '--localhost-port', - requestedPort: ports.localhost, - }, - ]) - }) - - describe('provideCertificate()', () => { - test('Calls renderInfo', async () => { - // Given - vi.mocked(checkPortAvailability).mockResolvedValue(true) - - // When - const mockOutput = mockAndCaptureOutput() - mockOutput.clear() - const result = (await getTunnelMode(defaultOptions)) as NoTunnel - await result.provideCertificate('app-directory') - - // Then - expect(mockOutput.info()).toContain('Localhost-based development is in developer preview') - }) - - test('Calls generateCertificate and returns its value', async () => { - // Given - vi.mocked(checkPortAvailability).mockResolvedValue(true) - - // When - const mockOutput = mockAndCaptureOutput() - mockOutput.clear() - const result = (await getTunnelMode(defaultOptions)) as NoTunnel - const certificate = await result.provideCertificate('app-directory') - - // Then - expect(generateCertificate).toHaveBeenCalledWith({ - appDirectory: 'app-directory', - onRequiresConfirmation: expect.any(Function), - }) - expect(certificate).toEqual(mockCertificate) - }) - }) -}) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index edeaa239ff..8375a0a9ce 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -26,6 +26,8 @@ import {canEnablePreviewMode} from './extensions/common.js' import {fetchAppRemoteConfiguration} from './app/select-app.js' import {patchAppConfigurationFile} from './app/patch-app-configuration-file.js' import {DevSessionStatusManager} from './dev/processes/dev-session/dev-session-status-manager.js' +import {TunnelMode} from './dev/tunnel-mode.js' +import {PortWarning, renderPortWarnings} from './dev/port-warnings.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {Web, isCurrentAppSchema, getAppScopesArray, AppLinkedInterface} from '../models/app/app.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' @@ -34,8 +36,6 @@ import {ports} from '../constants.js' import metadata from '../metadata.js' import {AppConfigurationUsedByCli} from '../models/extensions/specifications/types/app_config.js' import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' -import {generateCertificate} from '../utilities/mkcert.js' -import {generateCertificatePrompt} from '../prompts/dev.js' import {Config} from '@oclif/core' import {performActionWithRetryAfterRecovery} from '@shopify/cli-kit/common/retry' import {AbortController} from '@shopify/cli-kit/node/abort' @@ -43,41 +43,12 @@ import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/ import {TunnelClient} from '@shopify/cli-kit/node/plugins/tunnel' import {getBackendPort} from '@shopify/cli-kit/node/environment' import {basename} from '@shopify/cli-kit/node/path' -import {renderWarning, renderInfo, Token} from '@shopify/cli-kit/node/ui' +import {renderWarning} from '@shopify/cli-kit/node/ui' import {reportAnalyticsEvent} from '@shopify/cli-kit/node/analytics' import {OutputProcess, formatPackageManagerCommand, outputDebug} from '@shopify/cli-kit/node/output' import {hashString} from '@shopify/cli-kit/node/crypto' import {AbortError} from '@shopify/cli-kit/node/error' -export interface NoTunnel { - mode: 'use-localhost' - port: number - provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> -} - -export interface AutoTunnel { - mode: 'auto' -} - -export interface CustomTunnel { - mode: 'custom' - url: string -} - -type TunnelMode = NoTunnel | AutoTunnel | CustomTunnel -export type PortWarning = ( - | { - type: 'GraphiQL' - flag: '--graphiql-port' - } - | { - type: 'localhost' - flag: '--localhost-port' - } -) & { - requestedPort: number -} - export interface DevOptions { app: AppLinkedInterface remoteApp: OrganizationApp @@ -545,136 +516,3 @@ async function validateCustomPorts(webConfigs: Web[], graphiqlPort: number) { function setPreviousAppId(directory: string, apiKey: string) { setCachedAppInfo({directory, previousAppId: apiKey}) } - -/** - * Gets the tunnel or localhost config for doing app dev - * @param options - Options required for the config - * @returns A tunnel configuration object - */ -export async function getTunnelMode({ - useLocalhost, - localhostPort, - tunnelUrl, - portWarnings, -}: { - tunnelUrl?: string - useLocalhost?: boolean - localhostPort?: number - portWarnings: PortWarning[] -}): Promise { - // Developer brought their own tunnel - if (tunnelUrl) { - return {mode: 'custom', url: tunnelUrl} - } - - // CLI should create a tunnel - if (!useLocalhost && !localhostPort) { - return { - mode: 'auto', - } - } - - const requestedPort = localhostPort ?? ports.localhost - const actualPort = await getAvailableTCPPort(requestedPort) - - // The user specified a port. It's not available. Abort! - if (localhostPort && actualPort !== requestedPort) { - const errorMessage = `Port ${localhostPort} is not available.` - const tryMessage = ['Choose a different port for the', {command: '--localhost-port'}, 'flag.'] - throw new AbortError(errorMessage, tryMessage) - } - - // The user didn't specify a port. The default isn't available. Add to warnings array - // This will be rendered using renderWarning later when dev() is called - // This allows us to consolidate all port warnings into one renderWarning message - if (requestedPort !== actualPort) { - portWarnings.push({ - type: 'localhost', - requestedPort, - flag: '--localhost-port', - }) - } - - return { - mode: 'use-localhost', - port: actualPort, - provideCertificate: async (appDirectory) => { - renderInfo({ - headline: 'Localhost-based development is in developer preview.', - body: [ - '`--use-localhost` is not compatible with Shopify features which directly invoke your app', - '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', - 'device (such as POS). Please report any issues and provide feedback on the dev community:', - ], - link: { - label: 'Create a feedback post', - url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', - }, - }) - - return generateCertificate({ - appDirectory, - onRequiresConfirmation: generateCertificatePrompt, - }) - }, - } -} - -export function renderPortWarnings(portWarnings: PortWarning[] = []) { - if (portWarnings.length === 0 || !portWarnings[0]) return - - if (portWarnings.length === 1) { - const warning = portWarnings[0] - - renderWarning({ - headline: [`A random port will be used for ${warning.type} because ${warning?.requestedPort} is not available.`], - body: [ - `If you want to use a specific port, you can choose a different one by setting the `, - {command: warning?.flag}, - ` flag.`, - ], - }) - return - } - - const formattedWarningTypes = asHumanFriendlyTokenList(portWarnings.map((warning) => warning.type)).join(' ') - const formattedFlags = asHumanFriendlyTokenList(portWarnings.map((warning) => ({command: warning.flag}))) - - renderWarning({ - headline: [`Random ports will be used for ${formattedWarningTypes} because the requested ports are not available.`], - body: [`If you want to use specific ports, you can choose different ports using the`, ...formattedFlags, `flags.`], - }) -} - -/** - * Converts an array of Tokens into a human friendly list - * - * Returns a new array that contains the items separated by commas, - * except for the last item, which is seperated by "and". - * This is useful for creating human-friendly sentences. - * - * @example - * ```ts - * const items = ['apple', 'banana', 'cherry']; - * const result = asHumanFriendlyList(items) - * - * //['apple', ',', 'banana', ',', 'and', 'cherry'] - * console.log(result); - * ``` - */ - -function asHumanFriendlyTokenList(items: Token[]): Token[] { - if (items.length < 2) { - return items - } - - return items.reduce((acc, item, index) => { - if (index === items.length - 1) { - acc.push('and') - } else if (index !== 0) { - acc.push(', ') - } - acc.push(item) - return acc - }, []) -} diff --git a/packages/app/src/cli/services/dev/port-warnings.test.ts b/packages/app/src/cli/services/dev/port-warnings.test.ts new file mode 100644 index 0000000000..cbe77033ee --- /dev/null +++ b/packages/app/src/cli/services/dev/port-warnings.test.ts @@ -0,0 +1,66 @@ +import {PortWarning, renderPortWarnings} from './port-warnings.js' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' +import {describe, expect, test} from 'vitest' + +describe('renderPortWarnings()', () => { + test('does not call renderWarning when no warnings', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toBe('') + }) + + test('calls renderWarning when there is a warning', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: 1234, + }, + ] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toContain('A random port will be used for localhost because 1234 is not available.') + expect(mockOutput.warn()).toContain('If you want to use a specific port, you can choose a different one by') + expect(mockOutput.warn()).toContain('setting the `--localhost-port` flag.') + }) + + test('Combines warnings when there are multiple', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portWarnings: PortWarning[] = [ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: 1234, + }, + { + type: 'GraphiQL', + flag: '--graphiql-port', + requestedPort: 5678, + }, + ] + + // When + mockOutput.clear() + renderPortWarnings(portWarnings) + + // Then + expect(mockOutput.warn()).toContain('Random ports will be used for localhost and GraphiQL because the requested') + expect(mockOutput.warn()).toContain('ports are not available') + expect(mockOutput.warn()).toContain('If you want to use specific ports, you can choose different ports using') + expect(mockOutput.warn()).toContain('the `--localhost-port` and `--graphiql-port` flags.') + }) +}) diff --git a/packages/app/src/cli/services/dev/port-warnings.ts b/packages/app/src/cli/services/dev/port-warnings.ts new file mode 100644 index 0000000000..3f38c10e1b --- /dev/null +++ b/packages/app/src/cli/services/dev/port-warnings.ts @@ -0,0 +1,41 @@ +import {asHumanFriendlyArray} from '@shopify/cli-kit/common/array' +import {renderWarning} from '@shopify/cli-kit/node/ui' + +export type PortWarning = ( + | { + type: 'GraphiQL' + flag: '--graphiql-port' + } + | { + type: 'localhost' + flag: '--localhost-port' + } +) & { + requestedPort: number +} + +export function renderPortWarnings(portWarnings: PortWarning[] = []) { + if (portWarnings.length === 0 || !portWarnings[0]) return + + if (portWarnings.length === 1) { + const warning = portWarnings[0] + + renderWarning({ + headline: [`A random port will be used for ${warning.type} because ${warning?.requestedPort} is not available.`], + body: [ + `If you want to use a specific port, you can choose a different one by setting the `, + {command: warning?.flag}, + ` flag.`, + ], + }) + return + } + + const formattedWarningTypes = asHumanFriendlyArray(portWarnings.map((warning) => warning.type)).join(' ') + const formattedFlags = asHumanFriendlyArray(portWarnings.map((warning) => ({command: warning.flag}))) + + renderWarning({ + headline: [`Random ports will be used for ${formattedWarningTypes} because the requested ports are not available.`], + body: [`If you want to use specific ports, you can choose different ports using the`, ...formattedFlags, `flags.`], + }) +} diff --git a/packages/app/src/cli/services/dev/tunnel-mode.test.ts b/packages/app/src/cli/services/dev/tunnel-mode.test.ts new file mode 100644 index 0000000000..a0b02fc229 --- /dev/null +++ b/packages/app/src/cli/services/dev/tunnel-mode.test.ts @@ -0,0 +1,177 @@ +import {AutoTunnel, CustomTunnel, getTunnelMode, NoTunnel} from './tunnel-mode.js' +import {PortWarning} from './port-warnings.js' +import {generateCertificate} from '../../utilities/mkcert.js' +import {ports} from '../../constants.js' +import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' +import {beforeEach, describe, expect, test, vi} from 'vitest' +import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' + +describe('getTunnelMode() if tunnelUrl is defined', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: undefined, + portWarnings: [], + } + + test('returns AutoTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as AutoTunnel + + // Then + expect(result).toMatchObject({mode: 'auto'}) + }) +}) + +describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: 'https://my-tunnel-url.com', + portWarnings: [], + } + + test('returns CustomTunnel', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode(defaultOptions)) as CustomTunnel + + // Then + expect(result).toMatchObject({ + mode: 'custom', + url: defaultOptions.tunnelUrl, + }) + }) +}) + +describe('getTunnelMode() if useLocalhost is true', () => { + const mockCertificate = { + keyContent: 'test-key-content', + certContent: 'test-cert-content', + certPath: '/path/to/cert', + } + + beforeEach(() => { + vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) + }) + + const defaultOptions = { + useLocalhost: true, + localhostPort: undefined, + tunnelUrl: undefined, + portWarnings: [], + } + + test('returns localhostPort when passed', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + + // When + const result = (await getTunnelMode({ + ...defaultOptions, + localhostPort, + })) as NoTunnel + + // Then + expect(getAvailableTCPPort).toHaveBeenCalledWith(localhostPort) + expect(result).toMatchObject({ + mode: 'use-localhost', + port: localhostPort, + provideCertificate: expect.any(Function), + }) + }) + + test('throws when localhostPort is passed, but not available', async () => { + // Given + const localhostPort = 1234 + vi.mocked(getAvailableTCPPort).mockResolvedValue(4321) + + // Then + await expect( + getTunnelMode({ + ...defaultOptions, + localhostPort, + }), + ).rejects.toThrow() + }) + + test('returns ports.localhost when localhostPort is not passed', async () => { + // Given + vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost) + + // When + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + + // Then + expect(getAvailableTCPPort).toHaveBeenCalledWith(ports.localhost) + expect(result).toMatchObject({ + mode: 'use-localhost', + port: ports.localhost, + provideCertificate: expect.any(Function), + }) + }) + + test('Warns if ports.localhost is not available', async () => { + // Given + const availablePort = ports.localhost + 1 + vi.mocked(checkPortAvailability).mockResolvedValue(false) + vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) + const portWarnings: PortWarning[] = [] + + // When + const result = (await getTunnelMode({...defaultOptions, portWarnings})) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(result.port).toBe(availablePort) + expect(portWarnings).toStrictEqual([ + { + type: 'localhost', + flag: '--localhost-port', + requestedPort: ports.localhost, + }, + ]) + }) + + describe('provideCertificate()', () => { + test('Calls renderInfo', async () => { + // Given + vi.mocked(checkPortAvailability).mockResolvedValue(true) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + await result.provideCertificate('app-directory') + + // Then + expect(mockOutput.info()).toContain('Localhost-based development is in developer preview') + }) + + test('Calls generateCertificate and returns its value', async () => { + // Given + vi.mocked(checkPortAvailability).mockResolvedValue(true) + + // When + const mockOutput = mockAndCaptureOutput() + mockOutput.clear() + const result = (await getTunnelMode(defaultOptions)) as NoTunnel + const certificate = await result.provideCertificate('app-directory') + + // Then + expect(generateCertificate).toHaveBeenCalledWith({ + appDirectory: 'app-directory', + onRequiresConfirmation: expect.any(Function), + }) + expect(certificate).toEqual(mockCertificate) + }) + }) +}) diff --git a/packages/app/src/cli/services/dev/tunnel-mode.ts b/packages/app/src/cli/services/dev/tunnel-mode.ts new file mode 100644 index 0000000000..139eaea39c --- /dev/null +++ b/packages/app/src/cli/services/dev/tunnel-mode.ts @@ -0,0 +1,98 @@ +import {PortWarning} from './port-warnings.js' +import {ports} from '../../constants.js' +import {generateCertificate} from '../../utilities/mkcert.js' +import {generateCertificatePrompt} from '../../prompts/dev.js' +import {AbortError} from '@shopify/cli-kit/node/error' +import {getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' +import {renderInfo} from '@shopify/cli-kit/node/ui' + +export type TunnelMode = NoTunnel | AutoTunnel | CustomTunnel + +export interface NoTunnel { + mode: 'use-localhost' + port: number + provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> +} + +export interface AutoTunnel { + mode: 'auto' +} + +export interface CustomTunnel { + mode: 'custom' + url: string +} + +/** + * Gets the tunnel or localhost config for doing app dev + * @param options - Options required for the config + * @returns A tunnel configuration object + */ +export async function getTunnelMode({ + useLocalhost, + localhostPort, + tunnelUrl, + portWarnings, +}: { + tunnelUrl?: string + useLocalhost?: boolean + localhostPort?: number + portWarnings: PortWarning[] +}): Promise { + // Developer brought their own tunnel + if (tunnelUrl) { + return {mode: 'custom', url: tunnelUrl} + } + + // CLI should create a tunnel + if (!useLocalhost && !localhostPort) { + return { + mode: 'auto', + } + } + + const requestedPort = localhostPort ?? ports.localhost + const actualPort = await getAvailableTCPPort(requestedPort) + + // The user specified a port. It's not available. Abort! + if (localhostPort && actualPort !== requestedPort) { + const errorMessage = `Port ${localhostPort} is not available.` + const tryMessage = ['Choose a different port for the', {command: '--localhost-port'}, 'flag.'] + throw new AbortError(errorMessage, tryMessage) + } + + // The user didn't specify a port. The default isn't available. Add to warnings array + // This will be rendered using renderWarning later when dev() is called + // This allows us to consolidate all port warnings into one renderWarning message + if (requestedPort !== actualPort) { + portWarnings.push({ + type: 'localhost', + requestedPort, + flag: '--localhost-port', + }) + } + + return { + mode: 'use-localhost', + port: actualPort, + provideCertificate: async (appDirectory) => { + renderInfo({ + headline: 'Localhost-based development is in developer preview.', + body: [ + '`--use-localhost` is not compatible with Shopify features which directly invoke your app', + '(such as Webhooks, App proxy, and Flow actions), or those which require testing your app from another', + 'device (such as POS). Please report any issues and provide feedback on the dev community:', + ], + link: { + label: 'Create a feedback post', + url: 'https://community.shopify.dev/new-topic?category=shopify-cli-libraries&tags=app-dev-on-localhost', + }, + }) + + return generateCertificate({ + appDirectory, + onRequiresConfirmation: generateCertificatePrompt, + }) + }, + } +} diff --git a/packages/cli-kit/src/public/common/array.ts b/packages/cli-kit/src/public/common/array.ts index 8deecaee4e..0eaf56f0d8 100644 --- a/packages/cli-kit/src/public/common/array.ts +++ b/packages/cli-kit/src/public/common/array.ts @@ -67,3 +67,37 @@ export function uniqBy(array: List | null | undefined, iteratee: ValueIter export function difference(array: List | null | undefined, ...values: List[]): T[] { return lodashDifference(array, ...values) } + +/** + * Converts an array of anything into a human friendly list. + * + * Returns a new array that contains the items separated by commas, + * except for the last item, which is separated by "and". + * This is useful for creating human-friendly sentences. + * + * @param items - Token[]. + * @returns Token[]. + * @example + * ```ts + * const items = ['apple', 'banana', {command: "--flag"}]; + * const result = asHumanFriendlyList(items) + * + * // ['apple', ',', 'banana', ',', 'and', {command: "--flag"}] + * console.log(result); + * ``` + */ +export function asHumanFriendlyArray(items: T[]): (T | string)[] { + if (items.length < 2) { + return items + } + + return items.reduce<(T | string)[]>((acc, item, index) => { + if (index === items.length - 1) { + acc.push('and') + } else if (index !== 0) { + acc.push(', ') + } + acc.push(item) + return acc + }, []) +} From de5a22de2e7a92d35f21a4013d18d26b88c5533b Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Tue, 22 Apr 2025 12:03:55 -0400 Subject: [PATCH 10/12] PR Feedback: Don't pass an array to getTunnelMode() and dev(). Instead, getTunnelMode() returns the requested and actual port, which dev pased to renderPortWarnings(). renderPortWarnings() then renders errors every time requested and actual ports mis-match --- packages/app/src/cli/commands/app/dev.ts | 7 -- packages/app/src/cli/services/dev.ts | 42 ++++++----- .../cli/services/dev/port-warnings.test.ts | 73 ++++++++++++++----- .../app/src/cli/services/dev/port-warnings.ts | 31 ++++---- .../dev/processes/setup-dev-processes.test.ts | 5 -- .../src/cli/services/dev/tunnel-mode.test.ts | 69 ++++++++---------- .../app/src/cli/services/dev/tunnel-mode.ts | 20 +---- 7 files changed, 132 insertions(+), 115 deletions(-) diff --git a/packages/app/src/cli/commands/app/dev.ts b/packages/app/src/cli/commands/app/dev.ts index 0a9d879401..d7b654450d 100644 --- a/packages/app/src/cli/commands/app/dev.ts +++ b/packages/app/src/cli/commands/app/dev.ts @@ -5,7 +5,6 @@ import {checkFolderIsValidApp} from '../../models/app/loader.js' import AppCommand, {AppCommandOutput} from '../../utilities/app-command.js' import {linkedAppContext} from '../../services/app-context.js' import {storeContext} from '../../services/store-context.js' -import {PortWarning} from '../../services/dev/port-warnings.js' import {getTunnelMode} from '../../services/dev/tunnel-mode.js' import {Flags} from '@oclif/core' import {normalizeStoreFqdn} from '@shopify/cli-kit/node/context/fqdn' @@ -143,15 +142,10 @@ If you're using the Ruby app template, then you need to complete the following s await showApiKeyDeprecationWarning() } - // getTunnelMode can populate this array. This array is then passed to the dev function. - // This allows the dev function to group port warnings into a single renderWarning - const portWarnings: PortWarning[] = [] - const tunnelMode = await getTunnelMode({ useLocalhost: flags['use-localhost'], tunnelUrl: flags['tunnel-url'], localhostPort: flags['localhost-port'], - portWarnings, }) await addPublicMetadata(() => { @@ -191,7 +185,6 @@ If you're using the Ruby app template, then you need to complete the following s graphiqlPort: flags['graphiql-port'], graphiqlKey: flags['graphiql-key'], tunnel: tunnelMode, - portWarnings, } await dev(devOptions) diff --git a/packages/app/src/cli/services/dev.ts b/packages/app/src/cli/services/dev.ts index 8375a0a9ce..b8e3d63971 100644 --- a/packages/app/src/cli/services/dev.ts +++ b/packages/app/src/cli/services/dev.ts @@ -27,15 +27,15 @@ import {fetchAppRemoteConfiguration} from './app/select-app.js' import {patchAppConfigurationFile} from './app/patch-app-configuration-file.js' import {DevSessionStatusManager} from './dev/processes/dev-session/dev-session-status-manager.js' import {TunnelMode} from './dev/tunnel-mode.js' -import {PortWarning, renderPortWarnings} from './dev/port-warnings.js' +import {PortDetail, renderPortWarnings} from './dev/port-warnings.js' import {DeveloperPlatformClient} from '../utilities/developer-platform-client.js' import {Web, isCurrentAppSchema, getAppScopesArray, AppLinkedInterface} from '../models/app/app.js' import {Organization, OrganizationApp, OrganizationStore} from '../models/organization.js' import {getAnalyticsTunnelType} from '../utilities/analytics.js' -import {ports} from '../constants.js' import metadata from '../metadata.js' import {AppConfigurationUsedByCli} from '../models/extensions/specifications/types/app_config.js' import {RemoteAwareExtensionSpecification} from '../models/extensions/specification.js' +import {ports} from '../constants.js' import {Config} from '@oclif/core' import {performActionWithRetryAfterRecovery} from '@shopify/cli-kit/common/retry' import {AbortController} from '@shopify/cli-kit/node/abort' @@ -68,7 +68,6 @@ export interface DevOptions { notify?: string graphiqlPort?: number graphiqlKey?: string - portWarnings: PortWarning[] } export async function dev(commandOptions: DevOptions) { @@ -80,12 +79,13 @@ export async function dev(commandOptions: DevOptions) { } async function prepareForDev(commandOptions: DevOptions): Promise { - const {app, remoteApp, developerPlatformClient, store, specifications, portWarnings} = commandOptions + const {app, remoteApp, developerPlatformClient, store, specifications, tunnel} = commandOptions // Be optimistic about tunnel creation and do it as early as possible - const tunnelPort = await getAvailableTCPPort() let tunnelClient: TunnelClient | undefined - if (commandOptions.tunnel.mode === 'auto') { + + if (tunnel.mode === 'auto') { + const tunnelPort = await getAvailableTCPPort() tunnelClient = await startTunnelPlugin(commandOptions.commandConfig, tunnelPort, 'cloudflare') } @@ -121,23 +121,31 @@ async function prepareForDev(commandOptions: DevOptions): Promise { } const graphiqlPort = commandOptions.graphiqlPort ?? (await getAvailableTCPPort(ports.graphiql)) - const requestedGraphiqlPort = commandOptions.graphiqlPort ?? ports.graphiql - - if (graphiqlPort !== requestedGraphiqlPort) { - portWarnings.push({ - type: 'GraphiQL', - requestedPort: requestedGraphiqlPort, - flag: '--graphiql-port', + const portDetails: PortDetail[] = [ + { + for: 'GraphiQL', + flagToRemedy: '--graphiql-port', + requested: commandOptions.graphiqlPort ?? ports.graphiql, + actual: graphiqlPort, + }, + ] + + if (tunnel.mode === 'use-localhost') { + portDetails.push({ + for: 'localhost', + flagToRemedy: '--localhost-port', + requested: tunnel.requestedPort, + actual: tunnel.actualPort, }) } - renderPortWarnings(portWarnings) + renderPortWarnings(portDetails) const {webs, ...network} = await setupNetworkingOptions( app.directory, app.webs, graphiqlPort, - commandOptions.tunnel, + tunnel, tunnelClient, remoteApp.configuration, ) @@ -306,7 +314,7 @@ async function setupNetworkingOptions( tunnelOptions.mode === 'use-localhost' ? { noTunnelUseLocalhost: true, - port: tunnelOptions.port, + port: tunnelOptions.actualPort, } : { noTunnelUseLocalhost: false, @@ -339,7 +347,7 @@ async function setupNetworkingOptions( key: keyContent, cert: certContent, certPath, - port: tunnelOptions.port, + port: tunnelOptions.actualPort, } } diff --git a/packages/app/src/cli/services/dev/port-warnings.test.ts b/packages/app/src/cli/services/dev/port-warnings.test.ts index cbe77033ee..77e00babee 100644 --- a/packages/app/src/cli/services/dev/port-warnings.test.ts +++ b/packages/app/src/cli/services/dev/port-warnings.test.ts @@ -1,35 +1,68 @@ -import {PortWarning, renderPortWarnings} from './port-warnings.js' +import {PortDetail, renderPortWarnings} from './port-warnings.js' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' import {describe, expect, test} from 'vitest' describe('renderPortWarnings()', () => { - test('does not call renderWarning when no warnings', () => { + test('does not call renderWarning when no port details', () => { // Given const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [] + const portDetails: PortDetail[] = [] // When mockOutput.clear() - renderPortWarnings(portWarnings) + renderPortWarnings(portDetails) // Then expect(mockOutput.warn()).toBe('') }) - test('calls renderWarning when there is a warning', () => { + test('does not call renderWarning when request & actual ports match', () => { // Given const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [ + const portDetails: PortDetail[] = [ { - type: 'localhost', - flag: '--localhost-port', - requestedPort: 1234, + for: 'GraphiQL', + flagToRemedy: '--graphiql-port', + requested: 5678, + actual: 5678, + }, + { + for: 'localhost', + flagToRemedy: '--localhost-port', + requested: 1234, + actual: 1234, + }, + ] + + // When + mockOutput.clear() + renderPortWarnings(portDetails) + + // Then + expect(mockOutput.warn()).toBe('') + }) + + test('calls renderWarning once when there is one warning', () => { + // Given + const mockOutput = mockAndCaptureOutput() + const portDetails: PortDetail[] = [ + { + for: 'GraphiQL', + flagToRemedy: '--graphiql-port', + requested: 4321, + actual: 4321, + }, + { + for: 'localhost', + flagToRemedy: '--localhost-port', + requested: 1234, + actual: 4567, }, ] // When mockOutput.clear() - renderPortWarnings(portWarnings) + renderPortWarnings(portDetails) // Then expect(mockOutput.warn()).toContain('A random port will be used for localhost because 1234 is not available.') @@ -37,25 +70,27 @@ describe('renderPortWarnings()', () => { expect(mockOutput.warn()).toContain('setting the `--localhost-port` flag.') }) - test('Combines warnings when there are multiple', () => { + test('Calls renderWarning once, combining warnings when there are multiple warnings', () => { // Given const mockOutput = mockAndCaptureOutput() - const portWarnings: PortWarning[] = [ + const portDetails: PortDetail[] = [ { - type: 'localhost', - flag: '--localhost-port', - requestedPort: 1234, + for: 'localhost', + flagToRemedy: '--localhost-port', + requested: 4567, + actual: 7654, }, { - type: 'GraphiQL', - flag: '--graphiql-port', - requestedPort: 5678, + for: 'GraphiQL', + flagToRemedy: '--graphiql-port', + requested: 1234, + actual: 4321, }, ] // When mockOutput.clear() - renderPortWarnings(portWarnings) + renderPortWarnings(portDetails) // Then expect(mockOutput.warn()).toContain('Random ports will be used for localhost and GraphiQL because the requested') diff --git a/packages/app/src/cli/services/dev/port-warnings.ts b/packages/app/src/cli/services/dev/port-warnings.ts index 3f38c10e1b..49e997988e 100644 --- a/packages/app/src/cli/services/dev/port-warnings.ts +++ b/packages/app/src/cli/services/dev/port-warnings.ts @@ -1,38 +1,43 @@ import {asHumanFriendlyArray} from '@shopify/cli-kit/common/array' import {renderWarning} from '@shopify/cli-kit/node/ui' -export type PortWarning = ( +export type PortDetail = ( | { - type: 'GraphiQL' - flag: '--graphiql-port' + for: 'GraphiQL' + flagToRemedy: '--graphiql-port' } | { - type: 'localhost' - flag: '--localhost-port' + for: 'localhost' + flagToRemedy: '--localhost-port' } ) & { - requestedPort: number + requested: number + actual: number } -export function renderPortWarnings(portWarnings: PortWarning[] = []) { - if (portWarnings.length === 0 || !portWarnings[0]) return +export function renderPortWarnings(portDetails: PortDetail[]) { + if (!portDetails.length) return - if (portWarnings.length === 1) { + const portWarnings = portDetails.filter((warning) => warning.requested !== warning.actual) + + if (portWarnings.length === 0) return + + if (portWarnings.length === 1 && portWarnings[0]) { const warning = portWarnings[0] renderWarning({ - headline: [`A random port will be used for ${warning.type} because ${warning?.requestedPort} is not available.`], + headline: [`A random port will be used for ${warning.for} because ${warning?.requested} is not available.`], body: [ `If you want to use a specific port, you can choose a different one by setting the `, - {command: warning?.flag}, + {command: warning?.flagToRemedy}, ` flag.`, ], }) return } - const formattedWarningTypes = asHumanFriendlyArray(portWarnings.map((warning) => warning.type)).join(' ') - const formattedFlags = asHumanFriendlyArray(portWarnings.map((warning) => ({command: warning.flag}))) + const formattedWarningTypes = asHumanFriendlyArray(portWarnings.map((warning) => warning.for)).join(' ') + const formattedFlags = asHumanFriendlyArray(portWarnings.map((warning) => ({command: warning.flagToRemedy}))) renderWarning({ headline: [`Random ports will be used for ${formattedWarningTypes} because the requested ports are not available.`], diff --git a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts index 5c52d00eef..eec9cb080c 100644 --- a/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts +++ b/packages/app/src/cli/services/dev/processes/setup-dev-processes.test.ts @@ -95,7 +95,6 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, - portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -309,7 +308,6 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, - portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -382,7 +380,6 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, - portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -477,7 +474,6 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, - portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', @@ -562,7 +558,6 @@ describe('setup-dev-processes', () => { commandConfig: new Config({root: ''}), skipDependenciesInstallation: false, tunnel: {mode: 'auto'}, - portWarnings: [], } const network: DevConfig['network'] = { proxyUrl: 'https://example.com/proxy', diff --git a/packages/app/src/cli/services/dev/tunnel-mode.test.ts b/packages/app/src/cli/services/dev/tunnel-mode.test.ts index a0b02fc229..df4b38f373 100644 --- a/packages/app/src/cli/services/dev/tunnel-mode.test.ts +++ b/packages/app/src/cli/services/dev/tunnel-mode.test.ts @@ -1,23 +1,21 @@ import {AutoTunnel, CustomTunnel, getTunnelMode, NoTunnel} from './tunnel-mode.js' -import {PortWarning} from './port-warnings.js' import {generateCertificate} from '../../utilities/mkcert.js' import {ports} from '../../constants.js' import {checkPortAvailability, getAvailableTCPPort} from '@shopify/cli-kit/node/tcp' -import {beforeEach, describe, expect, test, vi} from 'vitest' +import {test, expect, describe, vi} from 'vitest' import {mockAndCaptureOutput} from '@shopify/cli-kit/node/testing/output' -describe('getTunnelMode() if tunnelUrl is defined', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: undefined, - portWarnings: [], - } +vi.mock('@shopify/cli-kit/node/tcp') +vi.mock('../../utilities/mkcert.js') +describe('getTunnelMode() if tunnelUrl is defined', () => { test('returns AutoTunnel', async () => { // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: undefined, + } // When const result = (await getTunnelMode(defaultOptions)) as AutoTunnel @@ -28,17 +26,13 @@ describe('getTunnelMode() if tunnelUrl is defined', () => { }) describe('getTunnelMode() if useLocalhost is false and tunnelUrl is a string', () => { - const defaultOptions = { - useLocalhost: false, - localhostPort: undefined, - tunnelUrl: 'https://my-tunnel-url.com', - portWarnings: [], - } - test('returns CustomTunnel', async () => { // Given - const localhostPort = 1234 - vi.mocked(getAvailableTCPPort).mockResolvedValue(1234) + const defaultOptions = { + useLocalhost: false, + localhostPort: undefined, + tunnelUrl: 'https://my-tunnel-url.com', + } // When const result = (await getTunnelMode(defaultOptions)) as CustomTunnel @@ -58,15 +52,14 @@ describe('getTunnelMode() if useLocalhost is true', () => { certPath: '/path/to/cert', } - beforeEach(() => { - vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) - }) + // beforeEach(() => { + // vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) + // }) const defaultOptions = { useLocalhost: true, localhostPort: undefined, tunnelUrl: undefined, - portWarnings: [], } test('returns localhostPort when passed', async () => { @@ -84,7 +77,8 @@ describe('getTunnelMode() if useLocalhost is true', () => { expect(getAvailableTCPPort).toHaveBeenCalledWith(localhostPort) expect(result).toMatchObject({ mode: 'use-localhost', - port: localhostPort, + requestedPort: localhostPort, + actualPort: localhostPort, provideCertificate: expect.any(Function), }) }) @@ -114,37 +108,35 @@ describe('getTunnelMode() if useLocalhost is true', () => { expect(getAvailableTCPPort).toHaveBeenCalledWith(ports.localhost) expect(result).toMatchObject({ mode: 'use-localhost', - port: ports.localhost, + actualPort: ports.localhost, + requestedPort: ports.localhost, provideCertificate: expect.any(Function), }) }) - test('Warns if ports.localhost is not available', async () => { + test('actualPort and requestedPort differ if ports.localhost is not available', async () => { // Given const availablePort = ports.localhost + 1 vi.mocked(checkPortAvailability).mockResolvedValue(false) vi.mocked(getAvailableTCPPort).mockResolvedValue(ports.localhost + 1) - const portWarnings: PortWarning[] = [] // When - const result = (await getTunnelMode({...defaultOptions, portWarnings})) as NoTunnel - await result.provideCertificate('app-directory') + const result = (await getTunnelMode(defaultOptions)) as NoTunnel // Then - expect(result.port).toBe(availablePort) - expect(portWarnings).toStrictEqual([ - { - type: 'localhost', - flag: '--localhost-port', - requestedPort: ports.localhost, - }, - ]) + expect(result).toMatchObject({ + mode: 'use-localhost', + actualPort: availablePort, + requestedPort: ports.localhost, + provideCertificate: expect.any(Function), + }) }) describe('provideCertificate()', () => { test('Calls renderInfo', async () => { // Given vi.mocked(checkPortAvailability).mockResolvedValue(true) + vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) // When const mockOutput = mockAndCaptureOutput() @@ -159,6 +151,7 @@ describe('getTunnelMode() if useLocalhost is true', () => { test('Calls generateCertificate and returns its value', async () => { // Given vi.mocked(checkPortAvailability).mockResolvedValue(true) + vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) // When const mockOutput = mockAndCaptureOutput() diff --git a/packages/app/src/cli/services/dev/tunnel-mode.ts b/packages/app/src/cli/services/dev/tunnel-mode.ts index 139eaea39c..749a0c7aba 100644 --- a/packages/app/src/cli/services/dev/tunnel-mode.ts +++ b/packages/app/src/cli/services/dev/tunnel-mode.ts @@ -1,4 +1,3 @@ -import {PortWarning} from './port-warnings.js' import {ports} from '../../constants.js' import {generateCertificate} from '../../utilities/mkcert.js' import {generateCertificatePrompt} from '../../prompts/dev.js' @@ -10,7 +9,8 @@ export type TunnelMode = NoTunnel | AutoTunnel | CustomTunnel export interface NoTunnel { mode: 'use-localhost' - port: number + actualPort: number + requestedPort: number provideCertificate: (appDirectory: string) => Promise<{keyContent: string; certContent: string; certPath: string}> } @@ -32,12 +32,10 @@ export async function getTunnelMode({ useLocalhost, localhostPort, tunnelUrl, - portWarnings, }: { tunnelUrl?: string useLocalhost?: boolean localhostPort?: number - portWarnings: PortWarning[] }): Promise { // Developer brought their own tunnel if (tunnelUrl) { @@ -61,20 +59,10 @@ export async function getTunnelMode({ throw new AbortError(errorMessage, tryMessage) } - // The user didn't specify a port. The default isn't available. Add to warnings array - // This will be rendered using renderWarning later when dev() is called - // This allows us to consolidate all port warnings into one renderWarning message - if (requestedPort !== actualPort) { - portWarnings.push({ - type: 'localhost', - requestedPort, - flag: '--localhost-port', - }) - } - return { mode: 'use-localhost', - port: actualPort, + requestedPort, + actualPort, provideCertificate: async (appDirectory) => { renderInfo({ headline: 'Localhost-based development is in developer preview.', From 3c7a7508614e1840746305006503463ac7bbe710 Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Tue, 22 Apr 2025 12:09:43 -0400 Subject: [PATCH 11/12] Ran pnpm run graphql-codegen --- .../business-platform-organizations/generated/types.d.ts | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts index 3c676bf56e..4e5d9ce544 100644 --- a/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts +++ b/packages/app/src/cli/api/graphql/business-platform-organizations/generated/types.d.ts @@ -19,6 +19,8 @@ export type Scalars = { AccessRoleRecordId: {input: any; output: any} /** The ID for a ActionAudit. */ ActionAuditID: {input: any; output: any} + /** The ID for a Address. */ + AddressID: {input: any; output: any} /** The ID for a BusinessUser. */ BusinessUserID: {input: any; output: any} /** A signed decimal number, which supports arbitrary precision and is serialized as a string. */ From c65b5bb91cdfbd0f71ff0211d53c4ac07af1b2ac Mon Sep 17 00:00:00 2001 From: Richard Powell Date: Wed, 23 Apr 2025 06:55:50 -0400 Subject: [PATCH 12/12] Remove commented code --- packages/app/src/cli/services/dev/tunnel-mode.test.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/packages/app/src/cli/services/dev/tunnel-mode.test.ts b/packages/app/src/cli/services/dev/tunnel-mode.test.ts index df4b38f373..3571cba5e8 100644 --- a/packages/app/src/cli/services/dev/tunnel-mode.test.ts +++ b/packages/app/src/cli/services/dev/tunnel-mode.test.ts @@ -52,10 +52,6 @@ describe('getTunnelMode() if useLocalhost is true', () => { certPath: '/path/to/cert', } - // beforeEach(() => { - // vi.mocked(generateCertificate).mockResolvedValue(mockCertificate) - // }) - const defaultOptions = { useLocalhost: true, localhostPort: undefined,