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
Show file tree
Hide file tree
Changes from all commits
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
Expand Up @@ -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"
},
Expand Down
27 changes: 20 additions & 7 deletions src/commands/dev/dev.js
Original file line number Diff line number Diff line change
Expand Up @@ -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'}`)
Expand All @@ -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`)
}

Expand All @@ -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
Expand Down Expand Up @@ -501,7 +510,11 @@ const dev = async (options, command) => {

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

const devCommand = () => startFrameworkServer({ settings })
const devCommand = async () => {
const { ipVersion } = await startFrameworkServer({ settings })
// eslint-disable-next-line no-magic-numbers
Copy link
Member

Choose a reason for hiding this comment

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

nit: I would actually create a constant for this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

IMHO I think it's easier to understand in the context of 6 ? '::1' : '127.0.0.1'

But I think it's personal preference just my 2 cents

settings.frameworkHost = ipVersion === 6 ? '::1' : '127.0.0.1'
}
const startDevOptions = getBuildOptions({
cachedConfig,
options,
Expand Down
6 changes: 3 additions & 3 deletions src/lib/http-agent.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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}'` }
}
Expand Down
10 changes: 7 additions & 3 deletions src/utils/proxy.js
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Expand Down Expand Up @@ -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,
},
})
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -498,6 +501,7 @@ const startProxy = async function ({
state,
})
const proxy = await initializeProxy({
host: settings.frameworkHost,
port: settings.frameworkPort,
distDir: settings.dist,
projectDir,
Expand Down
2 changes: 2 additions & 0 deletions src/utils/types.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion tests/integration/20.command.functions.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down