Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: Restore CLI port in use prompt feature #25863

Merged
merged 2 commits into from
Jul 21, 2020
Merged

fix: Restore CLI port in use prompt feature #25863

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ef48410
fix: Restore CLI port in use prompt feature
polarathene Jul 20, 2020
ba40cca
fix: Ensure port var types are the same
polarathene Jul 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions packages/gatsby/src/commands/develop.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import tmp from "tmp"
import { spawn, ChildProcess } from "child_process"
import chokidar from "chokidar"
import getRandomPort from "detect-port"
import { detectPortInUseAndPrompt } from "../utils/detect-port-in-use-and-prompt"
import socket from "socket.io"
import fs from "fs-extra"
import { isCI, slash } from "gatsby-core-utils"
Expand Down Expand Up @@ -163,6 +164,17 @@ const REGEX_IP = /^(?:(?:25[0-5]|2[0-4][0-9]|1[0-9][0-9]|[1-9]?[0-9])\.){3}(?:25

module.exports = async (program: IProgram): Promise<void> => {
const developProcessPath = slash(require.resolve(`./develop-process`))

try {
program.port = await detectPortInUseAndPrompt(program.port)
} catch (e) {
if (e.message === `USER_REJECTED`) {
process.exit(0)
}

throw e
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 looks like the same code from before

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. This PR is restoring that, as pointed out seems like it was accidentally removed.

// Run the actual develop server on a random port, and the proxy on the program port
// which users will access
const proxyPort = program.port
Expand Down
6 changes: 3 additions & 3 deletions packages/gatsby/src/utils/detect-port-in-use-and-prompt.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ import prompts from "prompts"
export const detectPortInUseAndPrompt = async (
port: number
): Promise<number> => {
let foundPort = port
const detectedPort = await detectPort(port).catch((err: Error) =>
let foundPort = typeof port === `string` ? parseInt(port, 10) : port
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you have to add this code? TS states that the port has to be a number.

Copy link
Contributor Author

@polarathene polarathene Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See last message I left in this PR. gatsby-cli types the port to string, it's unclear if that's intentional(it's possible to use ports that aren't numbers, though I doubt anyone is doing that). TS isn't doing any enforcement or conversion despite the typing.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah oof. Seems like a mistake on the TS side then.. Wonder if we should fix this more globally. to make port always be a number.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed up a change to handle the case early. Glad we found this discrepancy!

Copy link
Contributor Author

@polarathene polarathene Jul 21, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should be fine, do you want that change added to this PR?

EDIT: Nevermind, merged.

const detectedPort = await detectPort(foundPort).catch((err: Error) =>
report.panic(err)
)
if (port !== detectedPort) {
if (foundPort !== detectedPort) {
report.log(`\nSomething is already running at port ${port}`)
const response = await prompts({
type: `confirm`,
Expand Down