Add --localhost-port to port for --use-localhost flag #5630

byrichardpowell · 2025-04-09T19:45:59Z

WHY are these changes introduced?

We think controlling the localhost port is a very useful feature

WHAT is this pull request doing?

Changes the default behaviour for --use-localhost:

When--use-localhost flag is present, the CLI always tries to use the same port. Previously it was random
When --use-localhost flag is present but the default port is taken, it will take a random available port and warn the user

Adds a --localhost-port flag. When --localhost-port=7000 --use-localhost flags are present, the CLI will try to use port 7000. If it is not free, it will warn and take a random available port

I also did some refactors:

Moved logic from /commands/dev.ts into /services/dev.ts and tested that logic (Previously this was untested)
Updated types for services/dev/url.ts. Previously we had an interface with a bunch of optional properties. Now we have a union that more accurately describes when properties will be defined or undefined. This allowed me to delete some fallback code that was never required.

How to test your changes?

pnpm shopify app dev --path='[PATH]' should use a tunnel
Run pnpm shopify app dev --path='[PATH]' --use-localhost a few times. The port should not change
Run pnpm shopify app dev --path='[PATH]' --use-localhost in 2 separate terminals at once. The 2nd time you run it you should see a warning (see below) and a different port should be selected.
Run pnpm shopify app dev --path='[PATH]' --use-localhost --localhost=[PORT]. The port you passed should be used.
Run pnpm shopify app dev --path='[PATH]' --use-localhost --localhost=[PORT] in 2 separate terminals at once. The 2nd time you run it you should see a warning (see below) and a different port should be selected.

Post-release steps

Measuring impact

How do we know this change was effective? Please choose one:

n/a - this doesn't need measurement, e.g. a linting rule or a bug-fix
Existing analytics will cater for this addition
PR includes analytics changes to measure impact

Checklist

I've considered possible cross-platform impacts (Mac, Linux, Windows)
I've considered possible documentation changes

github-actions · 2025-04-10T13:38:37Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	76.71% (+0.04% 🔼)	9505/12391
🟡	Branches	71.83% (+0.06% 🔼)	4674/6507
🟡	Functions	76.47% (+0.02% 🔼)	2463/3221
🟡	Lines	77.24% (+0.04% 🔼)	8986/11634

Show new covered files 🐣

St.^❔	File	Statements	Branches	Functions	Lines
🟢	`...` / port-warnings.ts	100%	100%	100%	100%
🟢	`...` / tunnel-mode.ts	100%	100%	100%	100%

Show files with reduced coverage 🔻

St.^❔	File	Statements	Branches	Functions	Lines
🔴	`...` / dev.ts	18.66% (-0.28% 🔻)	14.08% (-0.41% 🔻)	24.39%	19.2% (-0.31% 🔻)
🟢	`...` / urls.ts	82.08% (-0.33% 🔻)	77.5%	75%	84% (-0.31% 🔻)
🔴	`...` / array.ts	43.75% (-56.25% 🔻)	0% (-100% 🔻)	77.78% (-22.22% 🔻)	40% (-60% 🔻)

Test suite run success

2217 tests passing in 966 suites.

Report generated by 🧪jest coverage report action from c65b5bb

byrichardpowell · 2025-04-10T14:19:50Z

packages/app/src/cli/services/dev/urls.test.ts


    // 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})


This change needs carefully reviewing. I think it's the correct test, and I think it hasn't changed behaviour because the previous test didn't match how this function was used. But we need to be sure.

Whoever reviews this please can you double check?

byrichardpowell · 2025-04-10T18:19:06Z

packages/app/src/cli/commands/app/dev.ts

@@ -91,11 +88,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)",


Changing since the previous list was non exhaustive. This new appraoch matches the approach we take elsewhere

byrichardpowell · 2025-04-10T18:42:27Z

packages/app/src/cli/commands/app/dev.ts


    await addPublicMetadata(() => {
      return {
        cmd_app_dependency_installation_skipped: flags['skip-dependencies-installation'],
        cmd_app_reset_used: flags.reset,
-        cmd_dev_tunnel_type: tunnelType,


Confirmed this value has not changed:

On this branch branch

pnpm shopify app dev --path='../app-test-use-localhost'
auto

pnpm shopify app dev --path='../app-test-use-localhost' --use-localhost
use-localhost

pnpm shopify app dev --path='../app-test-use-localhost' --tunnel-url="http://my-tunnel.com:3000"
custom

On main

pnpm shopify app dev --path='../app-test-use-localhost'
auto

pnpm shopify app dev --path='../app-test-use-localhost' --use-localhost
use-localhost

pnpm shopify app dev --path='../app-test-use-localhost' --tunnel-url="http://my-tunnel.com:3000"
custom

isaacroldan · 2025-04-11T08:30:38Z

Run pnpm shopify app dev --path='[PATH]' --use-localhost --localhost=[PORT] in 2 separate terminals at once. The 2nd time you run it you should see a warning (see below) and a different port should be selected.

I think this case should actually crash. If you are explicitly passing a setting (a specific port) and that port is not available it makes more sense to stop the execution than to ignore the setting

packages/app/src/cli/services/dev/urls.ts

packages/app/src/cli/services/dev.ts

packages/cli/oclif.manifest.json

byrichardpowell · 2025-04-11T15:10:06Z

I think this case should actually crash. If you are explicitly passing a setting (a specific port) and that port is not available it makes more sense to stop the execution than to ignore the setting

yeah, completely fair. I mis-understood how the --graphiql-port flag was working. Updated 👍

nickwesselman · 2025-04-11T19:23:09Z

/snapit

github-actions · 2025-04-11T19:24:57Z

🫰✨ Thanks @nickwesselman! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g @shopify/[email protected]

Tip

If you get an ETARGET error, install it with NPM and the flag --@shopify:registry=https://registry.npmjs.org

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

packages/app/src/cli/services/dev.ts

.changeset/clever-dogs-rush.md

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.

… service. Add tests

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

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()

* 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

packages/app/src/cli/services/dev.ts

isaacroldan · 2025-04-22T10:23:01Z

packages/app/src/cli/commands/app/dev.ts

+    // 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[] = []


I don't think you need to start this here and pass it to getTunnelMode and the dev service.

inside prepareForDev before calling renderPortWarnings you can just do:

if (tunnel.mode === 'use-localhost' && tunnel.port !== ports.localhost) { portWarnings.push({ type: 'localhost', requestedPort: tunnel.port, flag: '--localhost-port', }) }

* 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

…d, getTunnelMode() returns the requested and actual port, which dev pased to renderPortWarnings(). renderPortWarnings() then renders errors every time requested and actual ports mis-match

nickwesselman · 2025-04-22T16:38:04Z

/snapit

github-actions · 2025-04-22T16:40:09Z

🫰✨ Thanks @nickwesselman! Your snapshot has been published to npm.

Test the snapshot by installing your package globally:

pnpm i -g @shopify/[email protected]

Tip

If you get an ETARGET error, install it with NPM and the flag --@shopify:registry=https://registry.npmjs.org

Caution

After installing, validate the version by running just shopify in your terminal.
If the versions don't match, you might have multiple global instances installed.
Use which shopify to find out which one you are running and uninstall it.

isaacroldan · 2025-04-23T09:06:02Z

packages/app/src/cli/services/dev/tunnel-mode.test.ts

+  // beforeEach(() => {
+  //   vi.mocked(generateCertificate).mockResolvedValue(mockCertificate)
+  // })


remove this commented code please

github-actions · 2025-04-23T10:58:04Z

Differences in type declarations

We detected differences in the type declarations generated by Typescript for this branch compared to the baseline ('main' branch). Please, review them to ensure they are backward-compatible. Here are some important things to keep in mind:

Some seemingly private modules might be re-exported through public modules.
If the branch is behind main you might see odd diffs, rebase main into this branch.

New type declarations

We found no new type declarations in this PR

Existing type declarations

packages/cli-kit/dist/public/common/array.d.ts

@@ -45,4 +45,17 @@ export declare function uniqBy<T>(array: List<T> | null | undefined, iteratee: V
  * @param values - The arrays of values to exclude.
  * @returns Returns the new array of filtered values.
  */
-export declare function difference<T>(array: List<T> | null | undefined, ...values: List<T>[]): T[];
\ No newline at end of file
+export declare function difference<T>(array: List<T> | null | undefined, ...values: List<T>[]): T[];
+/**
+ * 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
+ * 
+ */
+export declare function asHumanFriendlyArray<T>(items: T[]): (T | string)[];
\ No newline at end of file

byrichardpowell requested a review from a team as a code owner April 9, 2025 19:45

This comment has been minimized.

Sign in to view

byrichardpowell force-pushed the specify-use-localhost-port branch 2 times, most recently from cb48f38 to 25ccd67 Compare April 10, 2025 13:30

byrichardpowell requested a review from a team as a code owner April 10, 2025 13:33

byrichardpowell commented Apr 10, 2025

View reviewed changes

byrichardpowell force-pushed the specify-use-localhost-port branch 5 times, most recently from 958ab68 to bc25a68 Compare April 10, 2025 18:17

byrichardpowell commented Apr 10, 2025

View reviewed changes

byrichardpowell force-pushed the specify-use-localhost-port branch from bc25a68 to 06eaa06 Compare April 10, 2025 18:28

byrichardpowell commented Apr 10, 2025

View reviewed changes

byrichardpowell changed the title ~~[WIP] Specify use localhost port~~ Add --localhost-port to port for --use-localhost flag Apr 10, 2025

isaacroldan reviewed Apr 11, 2025

View reviewed changes

packages/app/src/cli/services/dev/urls.ts Outdated Show resolved Hide resolved

isaacroldan reviewed Apr 11, 2025

View reviewed changes

packages/app/src/cli/services/dev.ts Outdated Show resolved Hide resolved

isaacroldan reviewed Apr 11, 2025

View reviewed changes

packages/app/src/cli/services/dev.ts Outdated Show resolved Hide resolved

nickwesselman reviewed Apr 11, 2025

View reviewed changes

packages/cli/oclif.manifest.json Outdated Show resolved Hide resolved

byrichardpowell force-pushed the specify-use-localhost-port branch 2 times, most recently from 89766ce to edcd74c Compare April 11, 2025 17:16

byrichardpowell requested review from isaacroldan and nickwesselman April 11, 2025 18:03

isaacroldan reviewed Apr 14, 2025

View reviewed changes

packages/app/src/cli/services/dev.ts Outdated Show resolved Hide resolved

isaacroldan reviewed Apr 14, 2025

View reviewed changes

.changeset/clever-dogs-rush.md Outdated Show resolved Hide resolved

byrichardpowell force-pushed the specify-use-localhost-port branch from 517a5d6 to 9ecfc35 Compare April 21, 2025 18:43

byrichardpowell added 5 commits April 21, 2025 14:53

Move tunnel logic to setup the tunnel out of the command and into the…

3a7d7b8

… service. Add tests

Ran pnpm refresh-manifests

9563130

Respond to PR Feedback

17590fc

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()

byrichardpowell force-pushed the specify-use-localhost-port branch from 9ecfc35 to fb1ba81 Compare April 21, 2025 18:53

byrichardpowell added 3 commits April 21, 2025 15:05

Added a changeset

7cdc129

PR Feedback:

31eb92a

* 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

Ran pnpm graphql-codegen

22d320f

byrichardpowell force-pushed the specify-use-localhost-port branch from 20abc74 to 22d320f Compare April 21, 2025 19:05

isaacroldan reviewed Apr 22, 2025

View reviewed changes

packages/app/src/cli/services/dev.ts Outdated Show resolved Hide resolved

isaacroldan reviewed Apr 22, 2025

View reviewed changes

packages/app/src/cli/services/dev.ts Outdated Show resolved Hide resolved

isaacroldan reviewed Apr 22, 2025

View reviewed changes

byrichardpowell force-pushed the specify-use-localhost-port branch from ca0216d to d719c89 Compare April 22, 2025 13:48

byrichardpowell added 2 commits April 22, 2025 12:03

PR Feedback: Don't pass an array to getTunnelMode() and dev(). Instea…

de5a22d

…d, getTunnelMode() returns the requested and actual port, which dev pased to renderPortWarnings(). renderPortWarnings() then renders errors every time requested and actual ports mis-match

Ran pnpm run graphql-codegen

3c7a750

nickwesselman approved these changes Apr 22, 2025

View reviewed changes

isaacroldan reviewed Apr 23, 2025

View reviewed changes

isaacroldan approved these changes Apr 23, 2025

View reviewed changes

Remove commented code

c65b5bb

byrichardpowell added this pull request to the merge queue Apr 23, 2025

Merged via the queue into main with commit c8e464a Apr 23, 2025
30 checks passed

byrichardpowell deleted the specify-use-localhost-port branch April 23, 2025 11:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --localhost-port to port for --use-localhost flag #5630

Add --localhost-port to port for --use-localhost flag #5630

byrichardpowell commented Apr 9, 2025 •

edited

Loading

This comment has been minimized.

github-actions bot commented Apr 10, 2025 •

edited

Loading

byrichardpowell Apr 10, 2025 •

edited

Loading

byrichardpowell Apr 10, 2025

byrichardpowell Apr 10, 2025

isaacroldan commented Apr 11, 2025

byrichardpowell commented Apr 11, 2025

nickwesselman commented Apr 11, 2025

github-actions bot commented Apr 11, 2025

isaacroldan Apr 22, 2025

nickwesselman commented Apr 22, 2025

github-actions bot commented Apr 22, 2025

isaacroldan Apr 23, 2025

github-actions bot commented Apr 23, 2025

Add --localhost-port to port for --use-localhost flag #5630

Add --localhost-port to port for --use-localhost flag #5630

Conversation

byrichardpowell commented Apr 9, 2025 • edited Loading

WHY are these changes introduced?

WHAT is this pull request doing?

How to test your changes?

Post-release steps

Measuring impact

Checklist

This comment has been minimized.

github-actions bot commented Apr 10, 2025 • edited Loading

Coverage report

Test suite run success

byrichardpowell Apr 10, 2025 • edited Loading

Choose a reason for hiding this comment

byrichardpowell Apr 10, 2025

Choose a reason for hiding this comment

byrichardpowell Apr 10, 2025

Choose a reason for hiding this comment

On this branch branch

On main

isaacroldan commented Apr 11, 2025

byrichardpowell commented Apr 11, 2025

nickwesselman commented Apr 11, 2025

github-actions bot commented Apr 11, 2025

isaacroldan Apr 22, 2025

Choose a reason for hiding this comment

nickwesselman commented Apr 22, 2025

github-actions bot commented Apr 22, 2025

isaacroldan Apr 23, 2025

Choose a reason for hiding this comment

github-actions bot commented Apr 23, 2025

Differences in type declarations

New type declarations

Existing type declarations

byrichardpowell commented Apr 9, 2025 •

edited

Loading

github-actions bot commented Apr 10, 2025 •

edited

Loading

byrichardpowell Apr 10, 2025 •

edited

Loading