Respond to PR Feedback

byrichardpowell · byrichardpowell · commit edcd74c525f8 · 2025-04-11T13:15:42.000-04:00
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()
diff --git 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
@@ -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
@@ -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
@@ -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<TunnelMode> {
-  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
@@ -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<FrontendURLResult> {
+  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
@@ -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,
