-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
feat: native support for Websockets #12973
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
base: main
Are you sure you want to change the base?
Conversation
…ionality for different handlers at different URLs, added example use cases to options-2 test app, added upgrade function for supporting additional adapters, and much more.
🦋 Changeset detectedLatest commit: d8d803f The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
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 |
preview: https://svelte-dev-git-preview-kit-12973-svelte.vercel.app/ this is an automated message |
@LukeHagar LMK, if you need any help with this (you can reach me also on discord |
I am testing this and have a problem with the production build using adapter-node. But publish and getPeers are supported by adapter-node, right? It is working fine with the dev server. {
"devDependencies": {
"@sveltejs/adapter-node": "https://pkg.pr.new/sveltejs/kit/@sveltejs/adapter-node@12973",
"@sveltejs/kit": "https://pkg.pr.new/sveltejs/kit/@sveltejs/kit@12973",
},
}
and then
|
Have you tried installing the latest version of this PR? (using the commit hash instead of the PR number) Otherwise, can you provide a minimal reproduction as a repository? |
Looking at the Cloudflare implemention it does not expose the power of durable object based websockets. These sockets are hibernatable meaning clients can stay connected at zero cost when no messages are being sent. People leave browser tabs open so this saves tons of money and make the code much simpler as you don't need to come up with a heuristic of when it is fine to auto disconnect people. I'd much prefer something lower level to this. The big problem for me is Sveltekit does not see wss:// requests. Exposing platform features is more important to me than than consistent crossplatform API. |
Hey @philholden, The implementation uses crossws under the hood, and I imagine any changes you think should be made could be made to get the support you want. Fancy having a look there to see how the implementation differs? |
they already support this in crossws https://crossws.h3.dev/adapters/cloudflare#durable-objects |
Then it should be as easy as adjusting the cloudflare adapter |
Im not sure how you imagine this btw. The Durable Objects is its own thing, not related to the Sveltekit app in any way. as long as you use Edit: actually no, the client dosent need to be aware of this what so ever from my understanding |
Thanks for reply and pointing out the crossws Durable Object (DO) adapter. I think the Hibernated DO is what is needed for most people using Cloudflare. Without it each CF Worker request spins up its own Worker at the edge.
Cloudflare’s docs call this out:
https://developers.cloudflare.com/workers/examples/websockets/ So I think configuring this in @sveltejs/adapter-cloudflare would work. The DO classes would need to be appended to _worker.js in the output. Either automatically or through a warning in the console the user would be prompted to add bindings for these in the wrangler.jsonc. I am trying to experiment with a modular full stack architecture in SvelteKit based on route folders. Where each folder is a vertical slice for a different functionality e.g. Posts, Images, Polls, Profiles, Wiki. Under that route you can have REST endpoints, WS, MCP (tools, prompts, resources), UI Editors and Viewers (ideally collaborative). These route folders behave like modules so you can compose an app by just dragging and dropping route folders into a project. If the design system is also in a route folder you could potentially swap out the design system too. Modules are just SvelteKit apps with a single route folder and a CLI tool can compose them. |
Thank you for making this! I really appreciate it! |
getPeers: ({ route }) => { | ||
// Return `true` if the production environment supports WebSockets, | ||
// return `false` if it can't. | ||
// Or throw a descriptive error describing how to configure the deployment | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more a note to self as i work through the PR than anything, but: the description here is identical to the one for socket
. Are there cases where one would be supported but not the other? If so it might be helpful if the descriptions explain the difference
```js | ||
/** @type {import('./$types').Socket} **/ | ||
export const socket = { | ||
open(peer) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be a deviation from the crossws API but if we used this signature instead we would be able to access the RequestEvent
without adding the event
property to the peer
object (which is also a deviation from the crossws API, but a more subtle one, and potentially a more breaky one if crossws were to later add an event
property of their own to peer
)
open(peer) { | |
open({ peer, event }) { |
Similarly for message
etc — could be message({ peer, message, event })
and so on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I love the experience this destructing provides, and love it today with the existing parts of kit
## Accessing `RequestEvent` through `Peer` | ||
|
||
The [`Peer`](https://crossws.unjs.io/guide/peer) object has been extended to include the [`RequestEvent`](@sveltejs-kit#RequestEvent) object from the initial upgrade request. It can be accessed through the `peer.event` property. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see prior note on tweaking the socket
signature rather than monkey-patching peer
## `getPeers` and `publish` | ||
|
||
The [`getPeers`]($app-server#getPeers) and [`publish`]($app-server#publish) functions from `$app/server` can be used to interact with your WebSocket connections from anywhere on the server. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't quite understand these functions — aren't they specific to a socket? publish
takes a topic
but it's not clear how I would subscribe to a topic in the first place
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok @eltigerchino has educated me here on how these work. My feeling is that we shouldn't expose getPeers
and publish
, because if getPeers
returns all peers for all socket
exports, then it becomes a source of bugs. Say I'm using getPeers
somewhere in my app and then another developers adds another +server.ts
with a socket
export. Suddenly I'll start broadcasting to the wrong peers.
Instead I think we should probably encourage people to handle peers manually, if you want to interact with them from outside the socket handlers:
// src/lib/topics/foo.ts
export const peers = new Set();
// src/routes/foo/+server.ts
import { peers } from '$lib/topics/foo';
export const socket = {
open({ peer }) {
peers.add(peer);
peer.send('hello');
},
close({ peer }) {
peers.delete(peer);
}
};
// src/routes/elsewhere/+server.ts
import { peers } from '$lib/topics/foo';
export async function POST({ request }) {
const message = await request.json();
for (const peer of peers) {
peer.send(message);
}
}
One question we're not too sure about: will this work with Durable Objects, or is there some magic for saving/restoring that we don't have access to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Durable objects have internal persistent state. As either Sqlite like db or KV. It also has in memory store. In example bellow I think sessions is in memory.
https://github.com/cloudflare/workers-chat-demo/blob/master/src/chat.mjs
Often I want to store state in the DO so new joiners get the current state from the DO the incremental state via WS.
So very helpful if DO can be exposed on platform.env.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Helpful if there is a general way to package DOs with Sveltekit through adapter-cloudflare as there are many nice DO things: MCP servers, partykit.
I'd really like to be able to put HelloWorld.durableObject.ts in a route folder and have it added to the worker output by the build.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@philholden see #13739
// @ts-ignore | ||
options | ||
); | ||
resolve_websocket_hooks = () => hooks; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a possible race condition here? If two upgrade requests came in at the same moment, could the resolve_websocket_hooks
called by the ws
instance's resolve
hook be (incorrectly) shared by the two requests?
It feels like if we had the ability to create the ws
instance here instead of sharing it between all requests, we'd be able to eliminate this risk and also reduce some of the indirection. But IIUC we can only call it once otherwise peers can't talk to each other
if (node.socket?.upgrade) { | ||
Object.defineProperty(event, 'context', { | ||
enumerable: true, | ||
value: context | ||
}); | ||
result = | ||
(await node.socket.upgrade( | ||
/** @type {import('@sveltejs/kit').RequestEvent & { context: {} }} */ (event) | ||
)) ?? undefined; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the upgrade handler should probably look like this, instead of adding event.context
:
/** @type {import('./$types').Socket} **/
export const socket = {
upgrade({ event, context }) {...}
};
This would match the change suggested in https://github.com/sveltejs/kit/pull/12973/files#r2073085655
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed above, this signature is a good experience IMO
Small input to the Durable Object thematic. No there shouldn't be any magic needed. a simple browser WebSocket can connect to it even with hibernation. BUT, due to websockets not supporting reconnecting out of the box, |
Hey everyone, little update here. A group of maintainers are having a team offsite ahead of Svelte Summit later this week, and we devoted much of today's session to talking about websockets. It was a great conversation and we had some realisations that would have been much harder to reach without the in-person aspect. We concluded that this PR makes some great headway BUT from an authoring perspective there's a bit of an impedance mismatch between the
We have some ideas we really like on what |
I appreciate the thought being put in here, and I can agree with all of the points on the interface and usage experience. And I'm very happy the conversation is ongoing :D Thanks for the update! |
Cool this is being worked on. Enjoy the summit.
I am just thinking about a notification counter that sits in the page header on all pages.
with or without requiring |
This really depends on your backend and usecase. Durable Objects are near perfect for scaling many kinds of collaborative apps. E.g. game servers, or an educational apps designed to serve a classroom where all the pupils are in the same location. The DO servers will end up getting automatically located at the edge near the users. There is no real penalty in terms of pricing for having one Durable Object per game or classroom instance. One of the problems I see for crossws is that while it did allow for a Durable Object backed DO. I think it is hard to make this work for different classes of DO in the same app E.g. the game server and the lobby server (each requires their own DO). The web socket adapter would need to be set up in https://crossws.h3.dev/adapters/cloudflare#durable-objects So for me there are two problems how to hear a |
Thanks for the feedback @Rich-Harris (just found about updates from a chat with @benmccann) Understandably, you might prefer to implement WebSockets standalone and design differently. I would love to have a chat to discuss more with svelekit team. There are a lot of compatibility details part from end user API that could be reused. Only to answer some of your concerns:
Peers only get notified about the topics they subscribe, but this is true; they are not isolated (between different resolved hook-sets). I had been talking with other folks before, and we might change that.
The global publish method is available on the main instance. Peers can auto-join topics mapped from their routes to have an API like
CrossWS also exposes an abstraction over SSE to give the same API: https://crossws.h3.dev/adapters/sse -- @philholden Re CF DO, we recently added new resolver option that gives full flexibility on routing upgrade request, to Durable instance. h3js/crossws#130 |
Hell to the yes about a << POST+SSE mechanism >> |
Thanks @pi0 the routing looks helpful. |
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
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
+server
exports validationPlease don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits