Skip to content

Native support for Websockets #12961

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

Closed
wants to merge 14 commits into from
Closed

Conversation

LukeHagar
Copy link
Contributor

@LukeHagar LukeHagar commented Nov 5, 2024

This PR is intended to add a path forward to natively support Websockets and similar protocols directly in SvelteKit by implementing an UPGRADE handler to be used similarly to existing GET, POST, HEAD +server.js exports

import { WebSocketServer } from 'ws';

const wss = new WebSocketServer({ noServer: true });

wss.on('connection', (ws) => {
	ws.on('error', console.error);

	ws.on('message', (message) => {
		console.log('received: %s', message);
	});
});

export function UPGRADE({ upgrade }) {
	wss.handleUpgrade(upgrade.request, upgrade.socket, upgrade.head, (ws) => {
		console.log('UPGRADED');
		wss.emit('connection', ws, upgrade.request);
	});
}

This PR will need to evolve to reflect the ideal implementation, and to correct things I have not yet like types, supports, etc...

Resolves Resolves #12358

Resolves #1491


Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

@LukeHagar LukeHagar marked this pull request as draft November 5, 2024 18:41
Copy link

changeset-bot bot commented Nov 5, 2024

⚠️ No Changeset found

Latest commit: 44e84ae

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@Rich-Harris
Copy link
Member

preview: https://svelte-dev-git-preview-kit-12961-svelte.vercel.app/

this is an automated message

@@ -54,9 +54,10 @@ const allowed_page_methods = new Set(['GET', 'HEAD', 'OPTIONS']);
* @param {import('types').SSROptions} options
* @param {import('@sveltejs/kit').SSRManifest} manifest
* @param {import('types').SSRState} state
* @returns {Promise<Response>}
* @param {{request: import('http').IncomingMessage, socket: import('stream').Duplex , head: Buffer}?} upgradeRequest
* @returns {Promise<void | Response>}
Copy link
Member

Choose a reason for hiding this comment

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

I'm not that familiar with UPGRADE, but is it true that it sends no response? A quick peek at MDN makes it appear to me that it does send a response

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Upgrade#overview

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There does appear to be some variance in the way the response is sent though, for example Bun does not send a response, but Deno does appear to return a standard HTTP response.

Socket.IO also returns a response but they refuse to just give you the resp object, and instead insist on binding directly to a Server so they can send it themselves.

If we look at the Node.JS server upgrade event, they specify the data is sent back through the socket, and their on upgrade handler does not even support sending a standard response object

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 would love maintainer/community input here, its definitely a fun issue to try and solve for everyone :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@LukeHagar LukeHagar Nov 6, 2024

Choose a reason for hiding this comment

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

I haven't had any luck getting this to work locally

Copy link
Member

@benmccann benmccann Nov 6, 2024

Choose a reason for hiding this comment

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

Oh sorry, I hadn't event noticed on first glance that UPGRADE is not an http verb like the others and it's an http header. I'm afraid I'm not terribly familiar with web sockets. I think we'll need to find another way of expressing this that doesn't mirror the API we use for HTTP verbs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Understood!

One thing that's worth mentioning is that this approach allows for different websocket handlers to be created on different routes, and for all other aspects of the requests and routing to use the existing SK response handling.

The incoming request is different, as the servers don't appear to pass these get requests through to middleware, so the existing middleware handling had to be somewhat copied to the server.on("upgrade") handler.

@Rich-Harris
Copy link
Member

Thank you, I'm excited for this functionality. I'm not quite sure this is the right API though. As @benmccann mentioned elsewhere, treating UPGRADE as an HTTP verb would be a little confusing.

I don't think it's just a naming issue though — I think this API is probably too low-level if I have to interact with ws myself, and too Node-centric. How would this work in the context of, say, Cloudflare Workers or Durable Objects or Bun or Deno? It might be good to start with some concrete use cases and design an API around that; it may not even make sense to expose the socket directly at all given that it takes a subtly different form on each platform. For context, here's how Nitro handles websockets.

@LukeHagar
Copy link
Contributor Author

Hey @Rich-Harris,

We are also discussing in the Svelte Discord here.

What do you think about exposing the hooks detailed here as a new export from hooks.server.js? This approach would allow us to easily add a new entry to the manifest/options alongside the handle and hook up each adapter as needed.

@LukeHagar
Copy link
Contributor Author

Here is an example of the above discussed solution.

This solution has some very specific differences which should be considered:

  1. The lack of unique ws handlers per request, which may have been a necessary tradeoff for broader support anyway
  2. The addition of an upgrade function similar to the current read function, this is intended to pass along the crossws instance along with any 'original' required request values that are provided by the serve function for different adapters (Bun, Deno, etc...)

That being said I don't think either of those things are bad, and I think that if we make the names of some variables or exports here a little more appealing (and get my bad types in order) this has the potential of being a very good solution.

@LukeHagar
Copy link
Contributor Author

I take it back you can do different paths using this feature of crossws

@LukeHagar LukeHagar changed the title Native support for Websockets, Webtransport by adding UPGRADE handler Native support for Websockets Nov 7, 2024
@loky-lp
Copy link

loky-lp commented Nov 7, 2024

What about exposing exposing an action-like object in +page.server.js/ts files with the API similar to how nitro does it?

Something like:

import type { Socket } from './$types';

export const socket = {
	open: (peer) => {
		peer.send({ user: "server,", message: `Welcome ${peer}!` });
		peer.publish("chat", { user: "server", message: `${peer} joined!` });
		peer.subscribe("chat");
	},
	message: (peer, message) => {
		if (message.text().includes("ping")) {
			peer.send({ user: "server", message: "pong" });
		} else {
			const msg = {
				user: peer.toString(),
				message: message.toString(),
			};
			peer.send(msg); // echo
			peer.publish("chat", msg);
		}
	},
	close: (peer) => {
		peer.publish("chat", { user: "server", message: `${peer} left!` });
	},
} satisfies Socket;

This way WebSockets can be created on a per-route basis and each step (open, close and message) could be properly handled by each platform with all of its shenanigans (👀 looking at you AWS lamba)

Or even better, maybe this object could be passed directly to crossws defineHook

@LukeHagar
Copy link
Contributor Author

Seems like implementing things this way instead would still be pretty straightforward, we would just need to change the root usage to be the shape crossws uses for routing, and do some work in the middle to actually load the correct paths websocket handlers.

@benmccann
Copy link
Member

What about exposing exposing an action-like object in +page.server.js/ts files with the API similar to how nitro does it?

Yeah, I think that's a nice idea.

Looks like Nitro imports a per-adapter implementation of crossws, which is something we could do too. E.g.:
https://github.com/nitrojs/nitro/blob/4307202f5aa5099e53b1653f382df393e929e086/src/presets/deno/runtime/deno-server.ts#L8
https://github.com/nitrojs/nitro/blob/4307202f5aa5099e53b1653f382df393e929e086/src/presets/bun/runtime/bun.ts#L5

I also noticed that Nitro marked their feature as experimental: https://nitro.build/guide/websocket#opt-in-to-the-experimental-feature, which is something we could consider as well if we want a bit of flexibility in changing the API.

@LukeHagar
Copy link
Contributor Author

Yes the current approach I am working on with crossws would I believe have the adapter import occurring at the SK adapter level. Allowing for the same approach.

@LukeHagar
Copy link
Contributor Author

Closing in favor of new PR #12973

@LukeHagar LukeHagar closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

implement "Upgrade" as a function in a api route to support websocket Native support for web sockets
4 participants