Implement websockets based on-demand-entries ping

[ijjk]

Fixes #4495

Here's my approach for replacing the XHR on-demand-entries pinger #1364 #4495. I'm not sure if this is the way everyone wants to accomplish this since I saw mention of using a separate server and port for the dynamic entries websocket, but thought this would be a fairly clean solution since it doesn't need that.

With this method the only change when using a custom server is you have to listen for the upgrade event and pass it to next.getRequestHandler(). Example:

const server = app.listen(port)
const handleRequest = next.getRequestHandler()

if(dev) {
  server.on('upgrade', handleRequest)
}

[ijjk]

Hey @rauchg I was wondering if I could get your feedback on this. Sorry to bother if you're the wrong person to ask, I thought I'd ask you since you prioritized the issue.

[timneutkens]

I'll review this soon, I'm currently implementing a new server.

[ijjk]

Okay cool, thanks for the reply!

[willopez]

@timneutkens can this be revisited?

[Enalmada]

Having to put a little code in custom server seems worth it. Are there any other disadvantages of this solution over the current one?

[icharge]

I'm waiting for this

[Enalmada]

Would it make reviewers more comfortable if this was configurable and people could opt-in to websocket based pinger? I would happily beta test this feature if there is some hesitation about making it the default right away.

[ijjk]

Hey @Enalmada I updated my dynamic-entries-websocket branch with the new next structure so if you want to give it a try it should be good to go.

[Enalmada]

@ijjk Thanks for taking the time to do this pull request. It seems to work great! Developers like me that spend lots of time with the chrome console open will be so happy when this is merged. It is a superior developer experience and has tests and everything.

[timneutkens]

Is there an alternative to doing this:

if(dev) {
  server.on('upgrade', handleRequest)
}

I assume we could open a socket server on a different (random) port in development?

This is the only part that makes me hesitate to merge this PR, as it'll break pretty much every implementation of Next.js that is currently out there, and it's easy to forget adding this.

[ijjk]

Yeah, that makes sense, it does seem better to avoid that. I updated my branch to create a WebSocket server on a random port for the dynamic-entries pinging in development.

[timneutkens]

💯 Going to have a look 👍

[timneutkens]

Let's put this on canary after my changes to render.js 💯

[ghost]

Is there a way to manually set the port? I have to manually map ports so this has corrupted my Dev experience. I did try setting process.env.NEXT_WS_PORT on both client and server.

[ijjk]

@glenarama currently you can't manually set the port for the on-demand-entries WebSocket. I just sent over a pull request adding a --websocket or -w argument similar to how the -p argument works to fix that.

[ghost]

Great, thanks!