kit icon indicating copy to clipboard operation
kit copied to clipboard

feat: native support for Websockets

Open LukeHagar opened this issue 1 year ago • 78 comments

This PR is a replacement to #12961 with a completely different Websocket implementation using crossws that should be fully compatible with all major runtimes.

Functionality has been validated locally using basic tests in the options-2 test app.

Here is the new usage experience. +server.js

import { error, accept } from '@sveltejs/kit';

export const socket = {
	upgrade(req) {
		 // Accept the websocket connection with a return
		return accept();

                // Reject the websocket connection with an error
                error(401, 'unauthorized');
	},

	open(peer) {
		//... handle socket open
	},

	message(peer, message) {
		//... handle socket message
	},

	close(peer, event) {
		//... handle socket close
	},

	error(peer, error) {
		//... handle socket error
	}
};

The newest implementation allows different sets of handlers to be implemented on a per-route basis. I have tested some basic uses of websockets locally to much success.

This PR is intended to: Resolve #12358 Resolve #1491

Steps left

  • [x] Ensure handle runs before upgrading requests
  • [x] Gather feedback
  • [x] Add or update tests
  • [x] Fix the types
  • [x] Update the adapters
  • [x] Update the documentation
  • [x] Add a changeset
  • [ ] Update language tools +server exports validation
  • [x] Automatic typing for sockets

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

  • [x] 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
  • [x] This message body should clearly illustrate what problems it solves.
  • [x] Ideally, include a test that fails without this PR but passes with it.

Tests

  • [x] Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • [x] 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

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

LukeHagar avatar Nov 08 '24 04:11 LukeHagar

🦋 Changeset detected

Latest commit: 24e84753752a360b0ed8ad3b1caee7fd737ec5ff

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@sveltejs/adapter-cloudflare Minor
@sveltejs/adapter-node Minor
@sveltejs/kit Minor
@sveltejs/adapter-auto Patch

Not sure what this means? Click here to learn what changesets are.

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

changeset-bot[bot] avatar Nov 08 '24 04:11 changeset-bot[bot]

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

this is an automated message

Rich-Harris avatar Nov 08 '24 04:11 Rich-Harris

@LukeHagar LMK, if you need any help with this (you can reach me also on discord @pi0)

pi0 avatar Jan 22 '25 11:01 pi0

@benmccann @dummdidumm @eltigerchino I think things are rapidly approaching done here, I would really appreciate if someone could lead the charge in getting any additional eyes on this needed, additionally if we could look to update the types however its needed to get the exports typed.

Thank you all so much for your help getting to this point!

I'm going to be rebasing things shortly

LukeHagar avatar Jan 24 '25 21:01 LukeHagar

There's some logic that's missing and might be important before the handle hook runs (mostly to do with populating the event object and whatever before_handle does). They're in the respond function but not the new resolve function. They might be needed because a user might rely on these in their handle function before their websocket handler runs

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L176-L177

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L242-L245

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L275

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L299-L322

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L330-L345

teemingc avatar Feb 04 '25 05:02 teemingc

There's some logic that's missing and might be important before the handle hook runs (mostly to do with populating the event object and whatever before_handle does). They're in the respond function but not the new resolve function. They might be needed because a user might rely on these in their handle function before their websocket handler runs

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L176-L177

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L242-L245

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L275

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L299-L322

https://github.com/sveltejs/kit/blob/461aa9584ee7a40e3c25a66be4768e98ec16cdd8/packages/kit/src/runtime/server/respond.js#L330-L345

I wrote resolve by copying respond completely and doing a number of incremental changes to get everything functional, there is a good chance that some things removed should actually be re-added, but I'm not familiar enough with those aspects of the internals to say one way or the other

LukeHagar avatar Feb 04 '25 17:02 LukeHagar

I have fully tested the node adapter and WS functionality through local builds.

LukeHagar avatar Feb 04 '25 21:02 LukeHagar

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

svelte-docs-bot[bot] avatar Feb 06 '25 02:02 svelte-docs-bot[bot]

I had actually added the supports implementation really early on but removed it during a refactor of the implementation because it was out of place, but I think there should be a very easy way to add it back in.

Thank you for all the notes Rich!

LukeHagar avatar Feb 11 '25 00:02 LukeHagar

Please ignore this question if you think it's irrelevant or off-topic. I was wondering how someone could integrate Socket.IO with this native web socket implementation. I know that will only work with the node-adapter, and I found https://socket.io/how-to/use-with-nuxt#hook-the-socketio-server for Nuxt, but I got a bit stuck when I needed to pass the node request/response object.

nerg4l avatar Feb 13 '25 11:02 nerg4l

Please ignore this question if you think it's irrelevant or off-topic. I was wondering how someone could integrate Socket.IO with this native web socket implementation. I know that will only work with the node-adapter, and I found https://socket.io/how-to/use-with-nuxt#hook-the-socketio-server for Nuxt, but I got a bit stuck when I needed to pass the node request/response object.

I'll leave a response to you on the issue so as not to cross wires with all the PR review happening here

benmccann avatar Feb 13 '25 22:02 benmccann

There are currently two pending issues which will cause the tests to fail until they are resolved:

  1. https://github.com/unjs/crossws/issues/141
  2. https://github.com/unjs/crossws/issues/139

Besides that, would love to get some feedback on the API, docs, tests, and anything else from user testing. You can try it out by installing it via:

# sveltekit
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/kit@98c2021

# node
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-node@98c2021

# cloudflare
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare@98c2021

# cloudflare workers
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare-workers@98c2021

teemingc avatar Feb 20 '25 05:02 teemingc

I'm probably missing something simple, but after installing with

pnpm i https://pkg.pr.new/sveltejs/kit/@sveltejs/kit@98c2021
pnpm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-node@98c2021

I get this error when testing in src/routes/+server.ts

Image

And when running anyway, this error:

TypeError: (0 , __vite_ssr_import_0__.accept) is not a function
    at Object.upgrade (E:\Projects\TEST\ws\src\routes\+server.ts:6:10)
    at resolve (E:\Projects\TEST\ws\node_modules\.pnpm\@sveltejs+kit@https+++pkg.p_4c8bcbcac749ac091b027133937142aa\node_modules\@sveltejs\kit\src\runtime\server\respond.js:483:44)
    at init_promise.#options.hooks.handle (E:\Projects\TEST\ws\node_modules\.pnpm\@sveltejs+kit@https+++pkg.p_4c8bcbcac749ac091b027133937142aa\node_modules\@sveltejs\kit\src\runtime\server\index.js:76:56)
    at upgrade (E:\Projects\TEST\ws\node_modules\.pnpm\@sveltejs+kit@https+++pkg.p_4c8bcbcac749ac091b027133937142aa\node_modules\@sveltejs\kit\src\runtime\server\respond.js:476:39)
    at hooks.upgrade (file:///E:/Projects/TEST/ws/node_modules/.pnpm/@sveltejs+kit@https+++pkg.p_4c8bcbcac749ac091b027133937142aa/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:573:8)
    at file:///E:/Projects/TEST/ws/node_modules/.pnpm/[email protected]/node_modules/crossws/dist/shared/crossws.D9ehKjSh.mjs:16:38
    at async AdapterHookable.upgrade (file:///E:/Projects/TEST/ws/node_modules/.pnpm/[email protected]/node_modules/crossws/dist/shared/crossws.D9ehKjSh.mjs:31:19)
    at async Object.handleUpgrade (file:///E:/Projects/TEST/ws/node_modules/.pnpm/[email protected]/node_modules/crossws/dist/adapters/node.mjs:59:56)
    at async Server.<anonymous> (file:///E:/Projects/TEST/ws/node_modules/.pnpm/@sveltejs+kit@https+++pkg.p_4c8bcbcac749ac091b027133937142aa/node_modules/@sveltejs/kit/src/exports/vite/dev/index.js:581:7)

So it's entering the upgrade function at least, but any ideas how to get the accept function to be avaliable?

Great to see this feature anyway. :)

ciscoheat avatar Feb 20 '25 18:02 ciscoheat

Hey @ciscoheat,

I think the accept function was removed in favor of just returning headers, These files should showcase how upgrades and errors should be handled for now

Upgrade: https://github.com/sveltejs/kit/pull/12973/commits/52677d7aa9c4cc1ce86e81d39421b71939494f0d#diff-ea6a6a78a1f81838fca612ef67430b4ae2b5f79946c9a9acebc5acc9cca9c19aR3-R7

Error: https://github.com/sveltejs/kit/pull/12973/commits/52677d7aa9c4cc1ce86e81d39421b71939494f0d#diff-4f62d3e799ec3b6382c37bef94a125df22cfda05a5ac458133250de86037aab2R5-R7

LukeHagar avatar Feb 20 '25 20:02 LukeHagar

Testing..

Why if i send the headers 'Sec-WebSocket-Protocol': 'test' exactly as the clients 'test' it disconects the client? it's the only bug i've got.

every time i test i get error on client, WebSocket connection to 'ws://localhost:5173/ws' failed:

however, if i do not send the headers, it continues working

alanxp avatar Feb 23 '25 04:02 alanxp

really hope they continue with this implementation. I’ve been testing it using ArrayBuffer and haven’t encountered any problems so far. Congrats to the devs—it looks promising! Do not give up!

alanxp avatar Feb 24 '25 04:02 alanxp

Why if i send the headers 'Sec-WebSocket-Protocol': 'test' exactly as the clients 'test' it disconects the client? it's the only bug i've got.

every time i test i get error on client, WebSocket connection to 'ws://localhost:5173/ws' failed:

however, if i do not send the headers, it continues working

Currently, there's an issue with returning the Sec-WebSocket-Protocol header because it is also returned by the crossws node adapter, causing the two to conflict. https://github.com/unjs/crossws/issues/141

teemingc avatar Feb 24 '25 04:02 teemingc

I want to handle message() outside of +server.ts using my own messageHandler implementation, but it does not let me import type { peer } from "@sveltejs/kit" outside of +server.ts, this is crucial to handle logic with modules.

also, is there anyway i can declare peer.context to the types i want instead of unknown?

alanxp avatar Feb 24 '25 15:02 alanxp

@alanxp

it does not let me import type { peer } from "@sveltejs/kit"

Peer is a type exported from crossws, but I don't think its one that's reexported. we can rectify that.

LukeHagar avatar Feb 25 '25 16:02 LukeHagar

@alanxp

it does not let me import type { peer } from "@sveltejs/kit"

Peer is a type exported from crossws, but I don't think its one that's reexported. we can rectify that.

I can confirm that it was a cache problem, it does work, so dont worry about it. Worked by deleting .sveltekit and restarting vscode + svelte dev server

Now what is really important, and it's not letting me continue my testing is the need to extend my types to peer.context

alanxp avatar Feb 25 '25 20:02 alanxp

Now what is really important, and it's not letting me continue my testing is the need to extend my types to peer.context

You can review the docs here: https://crossws.unjs.io/guide/peer

Though I don't see anything specific for typing context.

@pi0 Do you want to weigh in here, on how one could properly type peer.context or is this something we should look to solve in SvelteKit land?

luke-hagar-sp avatar Feb 25 '25 20:02 luke-hagar-sp

Now what is really important, and it's not letting me continue my testing is the need to extend my types to peer.context

Can you expand on your use case for this? It doesn't seem like peer.context contains any data unless explicitly set by the user. If you're adding your own data to the peer object, you could set it on a new property and modify the type like this:

// src/app.d.ts

// ...

declare module 'crossws' {
	export interface Peer {
		myData?: string; // augment the Peer object type with your own property
	}
}

export {};
// src/routes/server.ts
import type { Socket } from '@sveltejs/kit';

export const socket: Socket = {
  	open(peer) {
  	  peer.myData = 'my value'; // use your custom property
  	}
}

teemingc avatar Mar 03 '25 08:03 teemingc

I'm missing two functionalities which I think would be a great addition. Please let me know if these are already there and I just missed them.

  1. I did not find a way to access the adapters. With access to the adapter one can call publish outside of the hooks. See adapterUtils.
  2. Expose RequestEvent in the upgrade method. This would be quite useful to keep compatibility with Svelte Kit and not deviate from the functionality of other type of request/response flow. (e.g. access locals)

nerg4l avatar Mar 03 '25 09:03 nerg4l

I'm missing two functionalities which I think would be a great addition. Please let me know if these are already there and I just missed them.

1. I did not find a way to access the adapters. With access to the adapter one can call `publish` outside of the hooks. See [`adapterUtils`](https://github.com/unjs/crossws/blob/cad4a65b041d8f956067bded303a84f15e5278ef/src/adapter.ts#L4-L21).

2. Expose `RequestEvent` in the upgrade method. This would be quite useful to keep compatibility with Svelte Kit and not deviate from the functionality of other type of request/response flow. (e.g. access `locals`)

Thanks for your feedback! I've implemented these two changes and will test them over the next few days. The getPeers and publish functions can be imported from $app/server.

# sveltekit
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/kit@5e2bfc8

# node
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-node@5e2bfc8

# cloudflare
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare@5e2bfc8

# cloudflare workers
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare-workers@5e2bfc8

teemingc avatar Mar 05 '25 11:03 teemingc

I tried the changes and they are working so far. Now that I have access to RequestEvent in the upgrade method, I can access locals and it is nicely populated in hooks.server.ts. Unfortunately, this change removed the possibility of accessing the crossws context during upgrade, and I'm unable to share data with the hooks. Maybe svelte could populate context with locals? It feels a bit hackish. In my particular case, crossws Resolver API might be a better option but that probably out of scope for this PR.

Also, I noticed that I cannot simply send a SIGINT (CTRL+C) to shut down the node server while at least one WebSocket connection is alive. Do I have to register sveltekit:shutdown or something similar? It might be better to handle this on the adapter level.

nerg4l avatar Mar 07 '25 10:03 nerg4l

I tried the changes and they are working so far. Now that I have access to RequestEvent in the upgrade method, I can access locals and it is nicely populated in hooks.server.ts. Unfortunately, this change removed the possibility of accessing the crossws context during upgrade, and I'm unable to share data with the hooks. Maybe svelte could populate context with locals? It feels a bit hackish. In my particular case, crossws Resolver API might be a better option but that probably out of scope for this PR.

Also, I noticed that I cannot simply send a SIGINT (CTRL+C) to shut down the node server while at least one WebSocket connection is alive. Do I have to register sveltekit:shutdown or something similar? It might be better to handle this on the adapter level.

Thanks again! I've re-added the context property as event.context. I didn't realise people were using it to pass information between hooks.

We're already using the crossws resolver API under the hood.

I've also fixed the Node server not shutting down due to ongoing WebSocket connections. If you're manually handling the shutdown signal events, you'll need to import the helpers from handler.js to close all WebSocket connections.

Finally, I've temporarily added the patches for some crossws issues while we wait for the upstream fixes. You should be able to return websocket protocol headers during an upgrade now and the server shouldn't crash on Windows.

# sveltekit
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/kit@440e71c

# node
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-node@440e71c

# cloudflare
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare@440e71c

# cloudflare workers
npm i https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-cloudflare-workers@440e71c

teemingc avatar Mar 10 '25 11:03 teemingc

Any idea if supabase or vercel plan on supporting this out of the box?

rob-balfre avatar Mar 11 '25 02:03 rob-balfre

Any idea if supabase or vercel plan on supporting this out of the box?

No, I don't think they will. Supabase has its own realtime API that connects to their own servers while Vercel does not support WebSockets.

teemingc avatar Mar 11 '25 02:03 teemingc

I'm testing the websockets and it's well working, thank you for the work! Could you add "socket" to this list? https://github.com/sveltejs/kit/blob/831545515231849fc5b7b97ec7ff26d660e11bc8/packages/kit/src/utils/exports.js#L74 It's really not important but the warning is triggering me x)

PainOchoco avatar Mar 12 '25 16:03 PainOchoco

I'm testing the websockets and it's well working, thank you for the work! Could you add "socket" to this list?

https://github.com/sveltejs/kit/blob/831545515231849fc5b7b97ec7ff26d660e11bc8/packages/kit/src/utils/exports.js#L74

It's really not important but the warning is triggering me x)

It's worth mentioning that my branch does have this entry already

https://github.com/LukeHagar/kit/blob/1998d7b6de1c8e3e67a52d9f387a64d3683f5045/packages/kit/src/utils/exports.js#L74-L88

const valid_server_exports = new Set([
	'GET',
	'POST',
	'PATCH',
	'PUT',
	'DELETE',
	'OPTIONS',
	'HEAD',
	'fallback',
	'prerender',
	'trailingSlash',
	'config',
	'entries',
	'socket'
]);

luke-hagar-sp avatar Mar 12 '25 17:03 luke-hagar-sp