fix(deps): upgrade wait-port to v1 and listen on correct address for framework #5094
Conversation
danez
changed the title
📊 Benchmark resultsPackage size: 223 MBLegend
|
danez
force-pushed
the
frameworkport-node18
branch
from
1f53595 to
27eef3a
Compare
| @@ -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 | |||
There was a problem hiding this comment.
I'm assuming this is a typo?
| * @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 |
There was a problem hiding this comment.
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>} |
There was a problem hiding this comment.
The return value is always an empty object, right?
There was a problem hiding this comment.
Either empty or has the property ipVersion
| @@ -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 } | |||
There was a problem hiding this comment.
oooh, I like that we can finally use optional chaining in the CLI!
There was a problem hiding this comment.
yea because we dropped node 12 🥳
| const devCommand = () => startFrameworkServer({ settings }) | ||
| const devCommand = async () => { | ||
| const { ipVersion } = await startFrameworkServer({ settings }) | ||
| // eslint-disable-next-line no-magic-numbers |
There was a problem hiding this comment.
nit: I would actually create a constant for this.
There was a problem hiding this comment.
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
| host: '127.0.0.1', | ||
| host: 'localhost', |
There was a problem hiding this comment.
@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?
🎉 Thanks for submitting a pull request! 🎉
Summary
Updates wait-port to v1 which now tries both IPv6 and IPv4 and also reports where it found the open port. The proxy will then proxy to the correct address.
This should once and for all solve the IPv4 vs IPv6 problem with node 17+
Fixes #4845
Maybe helps with #3735 and #4942
For us to review and ship your PR efficiently, please perform the following steps:
passes our tests.
A picture of a cute animal (not mandatory, but encouraged)