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

Author: byrichardpowell

.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`
 

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)

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)

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<DevConfig> {
-  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<DevConfig> {
   }
 
   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<DevConfig> {
     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<TunnelMode> {
   // 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,26 +612,69 @@ 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,
       })
     },
   }
 }
+
+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<Token[]>((acc, item, index) => {
+    if (index === items.length - 1) {
+      acc.push('and')
+    } else if (index !== 0) {
+      acc.push(', ')
+    }
+    acc.push(item)
+    return acc
+  }, [])
+}