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.

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

fix(deps): upgrade wait-port to v1 and listen on correct address for framework #5094

Merged
merged 3 commits into from
Sep 30, 2022
Merged
Changes from 1 commit
Commits
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
14 changes: 7 additions & 7 deletions npm-shrinkwrap.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
@@ -326,7 +326,7 @@
"unixify": "^1.0.0",
"update-notifier": "^5.0.0",
"uuid": "^9.0.0",
"wait-port": "^0.3.0",
"wait-port": "^1.0.1",
"winston": "^3.2.1",
"write-file-atomic": "^4.0.0"
},
28 changes: 21 additions & 7 deletions src/commands/dev/dev.js
Original file line number Diff line number Diff line change
@@ -178,18 +178,25 @@ const runCommand = (command, env = {}, spinner = null) => {
return commandProcess
}

/**
* @typedef StartReturnObject
* @property {4 | 6 | undefined=} ipVersion The version the open port was found on
Copy link
Member

Choose a reason for hiding this comment

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

I'm assuming this is a typo?

Suggested change
* @property {4 | 6 | undefined=} ipVersion The version the open port was found on
* @property {4 | 6 | undefined} ipVersion The version the open port was found on

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No the equal sign marks the field as optional

*/

/**
* Start a static server if the `useStaticServer` is provided or a framework specific server
* @param {object} config
* @param {Partial<import('../../utils/types').ServerSettings>} config.settings
* @returns {Promise<void>}
* @returns {Promise<StartReturnObject>}
Copy link
Member

Choose a reason for hiding this comment

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

The return value is always an empty object, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Either empty or has the property ipVersion

*/
const startFrameworkServer = async function ({ settings }) {
if (settings.useStaticServer) {
if (settings.command) {
runCommand(settings.command, settings.env)
}
return await startStaticServer({ settings })
await startStaticServer({ settings })

return {}
}

log(`${NETLIFYDEVLOG} Starting Netlify Dev with ${settings.framework || 'custom config'}`)
@@ -200,17 +207,17 @@ const startFrameworkServer = async function ({ settings }) {

runCommand(settings.command, settings.env, spinner)

let port
try {
const open = await waitPort({
port = await waitPort({
port: settings.frameworkPort,
// Cannot use `localhost` as it may point to IPv4 or IPv6 depending on node version and OS
host: '127.0.0.1',
host: 'localhost',
Comment on lines -207 to +214

Choose a reason for hiding this comment

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

@danez @eduardoboucas this has caused a regressions for one of our own CI pipelines https://netlify.slack.com/archives/CPJQSPQE4/p1665509398672569

Could we use settings.frameworkHost here to solve this?

output: 'silent',
timeout: FRAMEWORK_PORT_TIMEOUT,
...(settings.pollingStrategies.includes('HTTP') && { protocol: 'http' }),
})

if (!open) {
if (!port.open) {
throw new Error(`Timed out waiting for port '${settings.frameworkPort}' to be open`)
}

@@ -221,6 +228,8 @@ const startFrameworkServer = async function ({ settings }) {
log(NETLIFYDEVERR, `Please make sure your framework server is running on port ${settings.frameworkPort}`)
exit(1)
}

return { ipVersion: port?.ipVersion }
Copy link
Member

Choose a reason for hiding this comment

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

oooh, I like that we can finally use optional chaining in the CLI!

Copy link
Collaborator

Choose a reason for hiding this comment

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

yea because we dropped node 12 πŸ₯³

}

// 10 minutes
@@ -414,6 +423,8 @@ const validateGeoCountryCode = (arg) => {
return arg.toUpperCase()
}

const IP_VERSION_6 = 6
danez marked this conversation as resolved.
Show resolved Hide resolved

/**
* The dev command
* @param {import('commander').OptionValues} options
@@ -501,7 +512,10 @@ const dev = async (options, command) => {

log(`${NETLIFYDEVWARN} Setting up local development server`)

const devCommand = () => startFrameworkServer({ settings })
const devCommand = async () => {
const { ipVersion } = await startFrameworkServer({ settings })
settings.frameworkHost = ipVersion === IP_VERSION_6 ? '::1' : '127.0.0.1'
}
const startDevOptions = getBuildOptions({
cachedConfig,
options,
6 changes: 3 additions & 3 deletions src/lib/http-agent.js
Original file line number Diff line number Diff line change
@@ -43,9 +43,9 @@ const tryGetAgent = async ({ certificateFile, httpProxy }) => {
return { error: `${httpProxy} must have a scheme of http or https` }
}

let open
let port
try {
open = await waitPort({
port = await waitPort({
port: Number.parseInt(proxyUrl.port) || (scheme === 'http' ? DEFAULT_HTTP_PORT : DEFAULT_HTTPS_PORT),
host: proxyUrl.hostname,
timeout: AGENT_PORT_TIMEOUT,
@@ -56,7 +56,7 @@ const tryGetAgent = async ({ certificateFile, httpProxy }) => {
return { error: `${httpProxy} is not available.`, message: error.message }
}

if (!open) {
if (!port.open) {
// timeout error
return { error: `Could not connect to '${httpProxy}'` }
}
10 changes: 7 additions & 3 deletions src/utils/proxy.js
Original file line number Diff line number Diff line change
@@ -4,6 +4,7 @@ const { once } = require('events')
const { readFile } = require('fs').promises
const http = require('http')
const https = require('https')
const { isIPv6 } = require('net')
const path = require('path')

const contentType = require('content-type')
@@ -281,11 +282,11 @@ const reqToURL = function (req, pathname) {

const MILLISEC_TO_SEC = 1e3

const initializeProxy = async function ({ configPath, distDir, port, projectDir }) {
const initializeProxy = async function ({ configPath, distDir, host, port, projectDir }) {
const proxy = httpProxy.createProxyServer({
selfHandleResponse: true,
target: {
host: '127.0.0.1',
host,
port,
},
})
@@ -434,7 +435,9 @@ const onRequest = async ({ addonsUrls, edgeFunctionsProxy, functionsServer, prox
const options = {
match,
addonsUrls,
target: `http://127.0.0.1:${settings.frameworkPort}`,
target: `http://${isIPv6(settings.frameworkHost) ? `[${settings.frameworkHost}]` : settings.frameworkHost}:${
settings.frameworkPort
}`,
publicFolder: settings.dist,
functionsServer,
functionsPort: settings.functionsPort,
@@ -498,6 +501,7 @@ const startProxy = async function ({
state,
})
const proxy = await initializeProxy({
host: settings.frameworkHost,
port: settings.frameworkPort,
distDir: settings.dist,
projectDir,
2 changes: 2 additions & 0 deletions src/utils/types.d.ts
Original file line number Diff line number Diff line change
@@ -24,6 +24,8 @@ export type BaseServerSettings = {
// Framework specific part
/** A port where a proxy can listen to it */
frameworkPort?: number
/** The host where a proxy can listen to it */
frameworkHost?: '127.0.0.1' | '::1'
functions?: string
/** The command that was provided for the dev config */
command?: string
2 changes: 1 addition & 1 deletion tests/integration/20.command.functions.test.js
Original file line number Diff line number Diff line change
@@ -660,7 +660,7 @@ const withFunctionsServer = async ({ builder, args = [], port = DEFAULT_PORT },
ps.stdout.on('data', (data) => console.log(data.toString()))
ps.stderr.on('data', (data) => console.log(data.toString()))

const open = await waitPort({
const { open } = await waitPort({
port,
output: 'silent',
timeout: SERVE_TIMEOUT,