fix: check ip version if settings.useStaticServer #6936
Conversation
khendrikse
requested review from
eduardoboucas
and removed request for
a team
src/utils/framework-server.ts
Outdated
| @@ -30,9 +31,11 @@ export const startFrameworkServer = async function ({ | |||
| if (settings.command) { | |||
| runCommand(settings.command, { env: settings.env, cwd }) | |||
| } | |||
| await startStaticServer({ settings }) | |||
| const { address } = await startStaticServer({ settings }) | |||
There was a problem hiding this comment.
Rather than getting address and then trying to figure out the IP family from the hostname, can you not use the family property directly?
There was a problem hiding this comment.
I'm gonna find out!!! yes!
No we didn't see it, but I'm going to try out running tests on win + node 23 in the pipelines. Wish me luck lol. |
khendrikse
force-pushed
the
CPLA-2390/expose-ip-version-for-static-server
branch
from
2710330
to
8a56858
Compare
khendrikse
force-pushed
the
CPLA-2390/expose-ip-version-for-static-server
branch
from
5322732
to
98ea076
Compare
| @@ -682,7 +682,9 @@ describe.concurrent('command/dev', () => { | |||
| async (server) => { | |||
| const output = server.outputBuffer.map((buff) => buff.toString()).join('\n') | |||
| t.expect(output).toContain('Server now ready') | |||
| t.expect(server.errorBuffer).toHaveLength(0) | |||
| // With node 23 we might be getting some warnings, but no errors | |||
There was a problem hiding this comment.
What warnings? Isn't that what we're fixing with this PR? Also, we're not looking for any specific OS or Node.js version here, so we're suddenly saying that it's okay for any OS and any Node.js version to have warnings — is that intended?
| runs-on: ${{ matrix.os }} | ||
| timeout-minutes: 40 | ||
| strategy: | ||
| matrix: |
There was a problem hiding this comment.
nit: You don't need a matrix if you only want to use one OS and one Node.js version.
| strategy: | ||
| matrix: | ||
| os: [windows-latest] | ||
| node-version: ['23'] |
| @@ -123,3 +123,105 @@ jobs: | |||
| flags: ${{ steps.test-coverage-flags.outputs.os }},${{ steps.test-coverage-flags.outputs.node }} | |||
| token: ${{ secrets.CODECOV_TOKEN }} | |||
| if: '${{ !steps.release-check.outputs.IS_RELEASE }}' | |||
| integration-win-node-23: | |||
There was a problem hiding this comment.
What is the long-term plan for this action? I understand we're looking to test this specific version today, but what happens after this goes to LTS and beyond? Can this be removed? Just trying to make sure we're not left with this extra step forever without people understanding why. Perhaps adding a comment here would help?
🎉 Thanks for submitting a pull request! 🎉
Summary
This hopefully fixes this: https://answers.netlify.com/t/128792
I noticed, while running this on windows in a VM, that the proxy was trying to connect to an IPv4 server, while the static server was actually an IPv6 one.
With this logic, we explicitly check what version we are running within the
settings.useStaticServerbranch of logic as well.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)