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

Additional config options for hot reloader web socket connection #6083

Merged

Conversation

jaslakson
Copy link
Contributor

We run Next on a sub-set of urls and the root url is not included in this group. Additionally, we run nginx over https as a proxy server for all urls, in all environments (local, staging, production, etc). The existing WebSocket configuration options are not sufficient for our needs.

Proposed changes:

  • conditionally set WebSocket protocol based on location.protocol (wss or ws)
  • add the following configuration options for onDemandEntries
    • websocketProxyPort: allows us to configure nginx to listen on one port and proxy pass to websocketProxyPort
    • websocketProxyPath: since the root of our site is not served by Next, we need a path to use in order to establish the socket connection

Tests:

I might need a little help here - looking at the existing ondemand tests, I wasn't sure where or how to test these options or if testing these options is completely necessary.

Thanks for all of your hard work on Next.js. My WebSocket experience is limited so I am open to feedback or pointers on how to do this differently.

I tried the steps outlined in your contributing guide but ran into errors when trying to test this in my local repository.

@@ -18,7 +18,9 @@ const defaultConfig = {
onDemandEntries: {
maxInactiveAge: 60 * 1000,
pagesBufferLength: 2,
websocketPort: 0
websocketPort: 0,
websocketProxyPath: null,
Copy link
Member

Choose a reason for hiding this comment

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

Should this instead just be taking assetPrefix into account instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so - at least in my case what I'm trying to accomplish here is set a unique path which corresponds to a location block in my nginx config. This allows the sw request to successfully establish itself.

Here's a snippet from my conf files so you can see what I am working with:

I am listening on 7001 and forwarding to 7002, which I have set as my websocketPort locally.

location /hmr {
  proxy_http_version 1.1;
  proxy_set_header Upgrade $http_upgrade;
  proxy_set_header Connection "upgrade";
  proxy_pass http://espresso_hot_server;
}
...elsewhere
upstream espresso_hot_server {
  server 127.0.0.1:7002;
}

@timneutkens
Copy link
Member

/home/circleci/repo/packages/next/server/hot-reloader.js:170:6: Extra semicolon.

Looks like linting didn't run.

@jaslakson
Copy link
Contributor Author

Just updated, removing the extra ; and confirmed linters run successfully locally.

@timneutkens
Copy link
Member

Could you add these new options to the readme too? After that + the changes I proposed I can merge the PR 👍

@timneutkens timneutkens merged commit 0000319 into vercel:canary Jan 19, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 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