analogjs / analog

The fullstack meta-framework for Angular. Powered by Vite and Nitro
https://analogjs.org
MIT License
2.56k stars 243 forks source link

Websocket Support #1398

Open nckirik opened 1 week ago

nckirik commented 1 week ago

Please provide the environment you discovered this bug in.

this is the repo i've tested the ws support: Analog Nitro Websocket

Which area/package is the issue in?

vite-plugin-nitro

Description

I've been working on getting WebSocket connections to work on AnalogJS for a while.

The Problem

In development environment, WebSockets close unexpectedly on the server side without informing the client. The hooks for upgrade and open in the /api/ws/chat event handler gets executed; however, the connection is then immediately closed with code 1006, causing the client to timeout.

In production environment, Websockets connections get established but there are no communication between the client and server.

Findings

I've found the root problem of this issue and wanted to share some thoughts and get some feedback before diving into a PR.

  1. Development Mode Upgrade Handling

    • In dev mode, vite-nitro-plugin attaches the Nitro app as middleware to Vite using
      viteServer.middlewares.use(
      apiPrefix,
      toNodeListener(server.app as unknown as App)
      );
    • This causes WebSocket upgrade requests to be treated as regular API requests (since there is no upgrade handler now), leading to the WebSocket hook for the requested route being called, but the socket connection itself never getting fully established. The connection looks like it's immediately closed on the server. But the client never gets notified.
    • To fix this, I made Vite's HTTP server pass the upgrade event back to Nitro:
      viteServer.httpServer?.on("upgrade", server.upgrade);
    • With this change, the WebSocket connection is successfully established, but the hook for the requested route still doesn't get called (same as in production).
  2. API Prefix

    • In dev, Vite handles all routes (Angular SSR and Nitro) and registers Nitro with an API prefix (by default /api). This makes server routes like /v1/hello available under /api/v1/hello.
    • In production, Nitro handles everything, and the apiMiddlewareHandler proxies prefixed routes to their appropriate handlers. However, this middleware doesn't get called for upgrade requests, which means routes like /api/ws/chat fail to find the WebSocket handler. The WebSocket works fine if accessed directly without the prefix (e.g., /ws/chat).
    • Nitro's routeRules doesn't affect this behavior either.
    • With the new upgrade handler proposed above, the behavior is same as in production. Websocket routes won't get the prefix. I think we can live without them for websockets, as long as we document the limitation and provide examples to avoid confusion
  3. Dev Environment HMR Conflicts

    • In dev mode, Vite uses the same port for HMR WebSocket as the dev server. With the new upgrade handler proposed above, it blocks Vite's own WebSocket connection, causing repeated reconnection attempts and a broken dev environment.
    • To avoid this conflict, we have two options;
      • Change Vite's HMR port by default in vite.config.ts in the templates
        export default defineConfig(({ mode }) => ({
        ...
        server: {
        hmr: {
          // different port than the dev server port
          // which is defined in project.json or angular.json
          port: 5001, 
          path: 'vite-hmr',
        },
        }
        ...
        });
      • Check if nitro.experimental.websocket is true to only apply the upgrade fix and document the need of HMR port change
        // Websockets are enabled, handle upgrade events
        if (nitroOptions?.experimental?.websocket) {
        viteServer.httpServer?.on("upgrade", server.upgrade);
        }
    • I think rather than changing the default port, we should check for experimenal websocket support to apply the upgrade handler. We should also document that the HMR Port should be changed if websocket support is desired.

Looking forward to your feedback!

Please provide the exception or error you saw

No response

Other information

No response

I would be willing to submit a PR to fix this issue

nckirik commented 1 week ago

One other minor finding I forgot to mention:

Nitro's upgrade handler currently accepts every request, even if there is no event handler for the requested route.

nckirik commented 1 day ago

hey @brandonroberts, shall i proceed for a PR like i suggested?

brandonroberts commented 1 day ago

@nckirik I think the conditional upgrade is fine.

I'm still a little confused on how the WebSocket will work with the apiPrefix?

Maybe we can add a dedicated path for WebSocket requests that's the same in development and production?

nckirik commented 11 hours ago

@nckirik I think the conditional upgrade is fine.

I'm still a little confused on how the WebSocket will work with the apiPrefix?

Maybe we can add a dedicated path for WebSocket requests that's the same in development and production?

the thing is, Nitro doesn't seem to apply any middleware/routeRules/dev proxy to upgraded websocket events. because of this apiPrefix is never applied to websocket routes.

in development, without the handling the upgrade event, all requests to websocket routes are handled like they are regular http requests so apiPrefix is applied. but after upgade fix they're handled same as production and apiPrefix wont work anymore.

so as you said, we can add a dedicated directory for websocket handlers like src/server/ws instead of src/server/routes and add this dir to scandirs. this will end up exposing /ws instead of /api/ws. but it wont prevent anyone from putting websocket handlers to src/server/routes and i dont think it's a problem.

brandonroberts commented 11 hours ago

Got it. I think just doing the conditional upgrade is the way to go then. We don't need to add the dedicated /ws if it doesn't add any value.