Rewrites are not working for websockets (socket.io)

marnixhoh commented 3 years ago

What version of Next.js are you using?

10.0.8

What version of Node.js are you using?

v12.13.1

What browser are you using?

Chrome

What operating system are you using?

macOS

How are you deploying your application?

next dev (development/local)

Describe the Bug

The rewrite functionality works great, however it appears to not support websockets (more specifically socket.io). Below is my rewrite config:

async rewrites() {
        return [
            {
                source: '/api/:slug*',
                destination: 'http://localhost:3000/api/:slug*'
            },
            {
                source: '/socket.io',
                destination: 'http://localhost:3000'
            }
        ]
    }

The fact that websockets are not supported by the rewrites, forces me to use a custom server for development. Because the docs recommend not to use a custom server as it will result in a loss of optimizations, I'd rather not use one. If any workarounds exist that do not involve setting up a custom server, I'd love to hear them! :D

Expected Behavior

Rewrites should support websockets, just as it support http requests

To Reproduce

See my rewrite config above.

marnixhoh commented 3 years ago

I looked at the Next.js source code and I believe, that supporting web sockets would be really easy. It seems like all that is required is to add the following line to next-server.ts:

const proxy = new Proxy({
  target,
  changeOrigin: true,
  ignorePath: true,
  ws: true // <-- this one
})

svetoslavstoyanov commented 3 years ago

Hi, use this https://github.com/vercel/next.js/tree/canary/examples/with-custom-reverse-proxy. It worked for me.

marnixhoh commented 3 years ago

@svetoslavstoyanov I ended up doing something similar. The problem with this solution though, is that it requires you to use a custom server. In production, I will use the default Next.js server to preserve all built-in optimizations. I prefer my production environment to be as similar as possible to my development environment to prevent any issues.

But thanks for the suggestion

jankaifer commented 1 year ago

Is this issue still relevant in next@canary? There is no reproduction that I can easily clone and check.

I looked at the Next.js source code and I believe, that supporting web sockets would be really easy.

That's great @marnixhoh. Send us a PR and we will check it out. There was already some work done in https://github.com/vercel/next.js/pull/31515 so you might want to check it out first.

marnixhoh commented 1 year ago

Hi @JanKaifer, I spent the morning investigating this and trying to see if this issue has been resolved in canary or not.

Since I opened this issue, my suggestion to add ws: true to the Proxy config has already been implemented. Note that all #31515 proposed, was to add this ws: true option.

Unfortunately, in the current canary version of Next, this turned out to not be enough to get socket.io rewrites to work.

So I dug a little deeper and I found the issue. The problem has to do with how Next formats the target of the rewrite rule. To get socket.io to work, the endpoint MUST have a trailing slash. E.g.:

✅ http://localhost:3000/socket.io/ ❌ http://localhost:3000/socket.io

Unfortunately, the Next server removes this trailing slash, resulting in a 404. I have tested forcing the target to have a trailing slash when it ends in /socket.io and this does indeed solve the problem and causes socket.io to work as expected.

Few things to note:

The trailingSlash option, which can be provided to next.config.js as documented here, does not seem to have any effect on this particular issue. Even when combined with the guidance found in the Rewrites documentation on trailing slashes:
```
// does not work:
async rewrites() {
return [
  {
    source: "/api/:slug*",
    destination: "http://localhost:3000/api/:slug*",
  },
  {
    source: "/socket.io/",
    destination: "http://localhost:3000/socket.io/",
  },
]
}
```


- There is a PR for `socket.io` to allow dropping the trailing slash: [see here](https://github.com/socketio/socket.io-client/issues/1550)

**How to proceed from here?**
I'd be more than happy to work on a PR to make `Rewrites` compatible with `socket.io`. This would involve creating some way for the user to indicate that the trailing slash should not be removed from a provided `target`. Perhaps trailing slashes should never be removed from a `target` or adding an additional `rewrite` option: `keepTrailingSlash` would do the trick?

Or of course, we can wait for the `socket.io` PR to go through.

@JanKaifer I'm looking forward to hearing what your thoughts are on this and how I can help. :)

jankaifer commented 1 year ago

Awesome. Thanks a lot, for digging into this.

I have tested forcing the target to have a trailing slash when it ends in /socket.io and this does indeed solve the problem and causes socket.io to work as expected.

Could you please mention how have you done this, so that others have a clear workaround?

The first step for fixing this would be to add a test for it. @timneutkens I've tried looking through the code/tests and I haven't found an easy way to test this. Can we require('http') and run a standalone server inside 2e2 tests?

marnixhoh commented 1 year ago

@JanKaifer Thanks for your quick comment!

I have done that by changing the next-server source code... It was just a quick and dirty check:

I changed this line next-server.ts:698:

const proxy = new HttpProxy({
      // target,
      target: target.startsWith('<url>/socket.io') ? target.replace('<url>/socket.io', '<url>/socket.io/') : target,
      changeOrigin: true,
      ignorePath: true,
      xfwd: true,
      ws: true,
      ....

So not much of a viable workaround for others, I'm afraid...

balazsorban44 commented 1 year ago

This issue has been automatically closed because it wasn't verified against next@canary. If you think it was closed by accident, please leave a comment. If you are running into a similar issue, please open a new issue with a reproduction. Thank you.

marnixhoh commented 1 year ago

I don't think it should be closed, as it was tested against next@canary. See this comment.

As I indicated in the same comment, I would love to create a PR to fix this. However, I will need some guidance from the maintainers as to what direction to take (see the last paragraph of the comment).

Thanks!

jankaifer commented 1 year ago

@marnixhoh Yup, thanks for pinging this. I forgot to remove the label and it was autoclosed. I'll ask relevant maintainers to help out here. But It might take a while since we have a lot of work with the app-directory and other new features.

B-Schmitz commented 9 months ago

same issue here, how i can resolve this?

solution: update socket io to 4.6.0 + add prop: addTrailingSlash: false,

FishyLipz1904 commented 4 months ago

This is now 2024 and it still hasn't worked. Has anyone came up with a solution?

Edit by maintainer bot: Comment was automatically minimized because it was considered unhelpful. (If you think this was by mistake, let us know). Please only comment if it adds context to the issue. If you want to express that you have the same problem, use the upvote 👍 on the issue description or subscribe to the issue for updates. Thanks!

rayrapetyan commented 3 months ago

Is anyone working on a fix? Not having a websockets proxy support in 2024 in the most popular server-side ReactJS framework is ridiculous...

timneutkens commented 1 week ago

As far as I could find this was already fixed a couple of months ago ago: https://github.com/vercel/next.js/pull/65759

Please open a new issue with a reproduction on the latest version if you have one.

vercel / next.js