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

Remove the second server.listen so that it also works with Node 20.13 #841

Merged
merged 1 commit into from
Jun 24, 2024

Conversation

yamachu
Copy link
Contributor

@yamachu yamachu commented May 30, 2024

ref: #829

Since Node 20.13, the behavior of Server.listen has changed.
The last listen will activate.
see: nodejs/node#53204

The SWA CLI used to listen for localhost and listen for localIpAdress (access from the same network), but in Node 18 and 20, the latter listen for localIpAdress does not seem to work anymore.
Only localhost listen is performed under 20.13.

Currently only the latter process is used in Node 20.13 environments, so only localIpAdress access is available.

Therefore, this PR will be adapted to the behavior under Node 20.13 so that it can be accessed by localhost and so on.

I think it is better to consider how to make it accessible via localIpAdress from the same network in a new PR.

Testing with this change can be verified with npm run e2e:static. (This is the e2e test that is currently disabled)

@github-actions github-actions bot added scope: msha Issues happened a the ./src/msha level status: need internal approval The issue need an internal approval from a stakeholder before it gets addressed labels May 30, 2024
@Timothyw0 Timothyw0 self-requested a review June 20, 2024 15:04
@Timothyw0
Copy link
Member

Timothyw0 commented Jun 20, 2024

@yamachu Thank you for looking into this issue and identifying the fix! I just tested in my local env with Node 18 and 20 and it's working great!

I will work with the code owners internally to get this PR reviewed and merged.

@IvanJobs
Copy link

IvanJobs commented Jun 21, 2024

Thanks for identifying and fixing this issue.

I'm wondering if there's some better solution for this. How about we make it listening on two endpoints for real by using two server instances?
Check https://www.quora.com/How-do-I-host-a-Node-js-app-that-listens-on-multiple-ports.

From the code, it seems trying to listen on two endpoints, but it turned out just listened on one of them which behaves differently by node versions.

@johnnyreilly
Copy link

Why would that be a better solution? If this fixes the issue then I'm very happy with that as a result!

@IvanJobs
Copy link

IvanJobs commented Jun 24, 2024

Why would that be a better solution? If this fixes the issue then I'm very happy with that as a result!

Just need to make sure it will not introduce another issue based on the original behavior which is listening on two endpoints.
To be robust, this can split into two steps:

  1. Keep the original behavior and fix the issue.
  2. If listening on two endpoints is useless, we can remove one of them in another PR.
    That's my opinion for reference.

If we're pretty sure, this can be done in this single PR.

Copy link

@IvanJobs IvanJobs left a comment

Choose a reason for hiding this comment

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

Confirmed with the original code owner and the team, it's good to just listen on one endpoint, so approve it.

@Timothyw0 Timothyw0 merged commit c30fd67 into Azure:main Jun 24, 2024
36 checks passed
@yamachu yamachu deleted the fix-server-listen branch June 24, 2024 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
scope: msha Issues happened a the ./src/msha level status: need internal approval The issue need an internal approval from a stakeholder before it gets addressed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants