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

Added WebSocket arg to allow manually setting port #5963

Merged
merged 7 commits into from
Jan 1, 2019

Conversation

ijjk
Copy link
Member

@ijjk ijjk commented Dec 30, 2018

Saw a reply on the original pull request that the WebSocket using a random port broke their set up so I added a --websocket or -w argument similar to the -p argument to allow manually setting this port also.

dynamic-entries websocket port
@timneutkens
Copy link
Member

Could someone that ran into this explain exactly why this wouldn't work? We're using Node.js defaults by listening on port 0. So it seems like the correct default / the user shouldn't need to worry about this 🤔

@timneutkens
Copy link
Member

Potentially because of a docker-compose type environment I guess?

@ijjk
Copy link
Member Author

ijjk commented Dec 30, 2018

Yeah, I would think using a random port would work for most setups using Next. Although being able to manually map it could be good in case anyone keeps their environment's firewall locked down on a port basis or they are using a docker-compose environment as you said.

@glenarama could you explain a little more your use case for manually mapping the WebSocket port?

@ghost
Copy link

ghost commented Dec 30, 2018

I'm using a Chromebook, it has a Linux container which I use for Dev.
In order to do local in browser testing/dev I need to open up specific ports. Currently the page reloads every few seconds because the port is blocked.

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

I think this should only be a next.config.js option. I'm not sure about the name though 🤔 devWebsocketPort 🤔

@ijjk
Copy link
Member Author

ijjk commented Dec 30, 2018

The devWebSocketPort name seems to make sense, I just pushed up that change.

@timneutkens
Copy link
Member

@ijjk could you add a section to the readme explaining that by default there is no need to configure this, but if you do you can do it by setting devWebSocketPort

@timneutkens
Copy link
Member

Could you also add a test for this if possible. There's a test suite for config options in test/integration/config

@ijjk
Copy link
Member Author

ijjk commented Dec 30, 2018

I added a note in the README under "Configuring the onDemandEntries" let me know if that's not the right place for it. I also added a test in the suite you mentioned. I had to dynamically write to the next.config.js for that test suite since there is the possibility of the port I define in the config being unavailable.

// create dynamic entries WebSocket
this.wss = new WebSocket.Server({ port: 0 }, function (err) {
this.wss = new WebSocket.Server({ port: devWebSocketPort || 0 }, function (err) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's always read from devWebsocketPort and add 0 as a default in packages/next-server/server/config.js` Then we have a single source of truth 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, I updated it

}
},
// optionally configure a port for the onDemandEntries WebSocket, not needed by default
devWebSocketPort: 3001,
Copy link
Member

Choose a reason for hiding this comment

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

I just realized we have a onDemandEntries key, totally forgot about it 🤦‍♂️ Can you move devWebSocketPort under onDemandEntries and call it websocketPort 🕵️

Copy link
Member

Choose a reason for hiding this comment

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

Then I'll merge 💯

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

As per my updated comment, sorry for changing it yet again 😅

@ijjk
Copy link
Member Author

ijjk commented Dec 31, 2018

It's all good, that seems like a better place for it. I also moved the onDemandEntries config to the defaultConfig to keep that as the source of truth.

@ghost
Copy link

ghost commented Dec 31, 2018

Thanks guys, really appreciate it!!

Copy link
Member

@timneutkens timneutkens left a comment

Choose a reason for hiding this comment

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

Awesome thanks so much 🙏 Happy new year!

@timneutkens timneutkens merged commit ba8cb31 into vercel:canary Jan 1, 2019
@ijjk
Copy link
Member Author

ijjk commented Jan 1, 2019

Happy New Year to you too!

@ijjk ijjk deleted the manual-websocket-port branch February 26, 2019 16:31
@lock lock bot locked as resolved and limited conversation to collaborators Feb 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants