refactor: unify sse endpoints handling #950

bjoerge · 2025-01-03T13:21:21Z

This needs a bit of a close look, but tldr; it refactors our current EventSource/SSE handling and unifies it under a common connectEventsource() function, now shared between the Live Content API and the Listener API. This function handles Sanity API SSE endpoint specific behaviors like dealing with channelError, disconnect event etc, but should be flexible enough to adapt to potential new future SSE endpoints.

The connectEventsource() function takes in a initEventSource function that allows us to abstract out the actual eventsource instantiation, which tends to vary depending on request options (headers) and environments.

Here's an example of how live query api uses it to determine which eventsource implementation to use:

client/src/data/listen.ts

Line 96 in b07b8d6

const initEventSource = () =>

This will lazy load the polyfill if either 1) no global EventSource is defined or 2) if headers are passed. Otherwise it will use the native EventSource class provided by the runtime.

I've also formalized the error classes and documented what errors can be thrown on the observable and under what circumstances they will be thrown. This should enable downstream consumers to better reason about and recover from the different types of errors.

I feel fairly confident that this patch doesn't introduce any breaking changes (beyond the bugfix mention below). Please tell me if I've overlooked something else.

This also exports the standalone connectEventsource() and the different error classes/helper types changes, which enables internal usage (tagged as @internal for now).

Backwards incompatible change with regards to `disconnect` events

Currently, if the server sends a disconnect event, the listener stream will complete. This is from main:

client/src/data/listen.ts

Line 133 in 5f0a991

observer.complete()

This seems like a bug with a potential for unfortunate effects. Consider the following:

A listener is set up to receive patches for a certain document. The returned observable emits mutations and the application use these to keep a local version of the document in sync.
The backend sends a disconnect event, which causes the observable to complete, and never reconnect.
No error is thrown and the application is not likely to pick up on the error and continue as if nothing happened, leaving the document out of sync.

I'd argue that this is a bug, and that the observable here should be kept "open" forever until the subscriber unsubscribes, and that a disconnect event should be treated as an error condition. Now, it looks like the only cases where a disconnect is actually sent are cases where the dataset has been deleted or similar exceptional cases, but I'd argue that throwing an error on the observable in these cases still would be preferable, and doing so is not a breaking change, but should be considered a bugfix with the reasoning that if it happens, we actually want it to raise an error instead of silently fail.

bjoerge · 2025-01-03T13:27:02Z

src/data/eventsource.ts

+  events: EventTypeName[],
+) {
+  return new Observable<EventSourceEvent<EventTypeName>>((observer) => {
+    const emitOpen = (events as string[]).includes('open')


this now makes it possible to subscribe to open events as well (but opt-in)

bjoerge · 2025-01-03T13:29:43Z

src/data/eventsourcePolyfill.ts

+import {defer, shareReplay} from 'rxjs'
+import {map} from 'rxjs/operators'
+
+export const eventSourcePolyfill = defer(() => import('@sanity/eventsource')).pipe(


this will lazy load the polyfill, share it between consumers and cache it forever

Really cool! And so much more elegant than the old new Promise song and dance 🤯

this refactors our current EventSource/SSE handling and unifies it under a common implementation shared between the live content api and the listener api.

…ct messages

bjoerge · 2025-01-03T16:05:12Z

test/listen.test.ts

@@ -74,16 +74,12 @@ describe.skipIf(typeof EdgeRuntime === 'string' || typeof document !== 'undefine
        ({request, channel}) => {
          expect(request.headers.authorization, 'should send token').toBe('Bearer foobar')

-          channel!.send({event: 'disconnect'})
-          process.nextTick(() => {
-            channel!.close()


closing here will trigger an error/reconnecting state, so not needed since we only take the welcome event from the observable

bjoerge · 2025-01-03T16:06:27Z

test/listen.test.ts

-        channel!.close()
-        process.nextTick(() => channel!.close())


also not needed since an error thrown on the observable will call the observable cleanup, which will do the closing for us

sgulseth

nice, code wise lgtm! some nits around typings 🤷 Would be nice to get either @rexxars or @stipsan feedback as well, as i'm not that familiar with the client

sgulseth · 2025-01-08T09:07:57Z

src/types.ts

+ * note: it's usually a better option to use the 'welcome' event
+ * @public
+ */
+export type OpenEvent = {


Should this be part of ListenEvent on L878?

Good catch! Will add it

sgulseth · 2025-01-08T09:09:47Z

src/data/eventsource.ts

+ *
+ * @internal
+ */
+export function connectEventSource<EventName extends string>(


nit:

Suggested change

export function connectEventSource<EventName extends string>(

const EventTypeName = ListenEvent['type']

export function connectEventSource<EventName extends EventTypeName[]>(

should make this type safe? 🤔 and same on L123?

It is type safe… kind of. Here we can only really know for sure that the event matches any of the event names given, which is why this returns {type: Name, id:? string, data?: unknown}. I left it up to the endpoints themselves (listen() and live() – for now) to take care of typing it correctly, since they have more specific knowledge of the payloads. I also made a choice not to splat data into the events at this level, because in theory it could contain type and id which would lead to very surprising behavior.

stipsan

Absolute brilliant 💖 I always learn so much about rxjs when reviewing your PRs, and your improvements are so elegant :chefs-kiss:

stipsan · 2025-01-08T09:57:53Z

src/data/live.ts

What do you think about using a shareReplay cache trick like the polyfill here?
The cache key can be: JSON.stringify({url: url.href, esOptions}) and it would allow interesting improvements to Sanity Live. Unlike client.listen(), the resulting URL doesn't have filters or anything else that lead to many unique URLs. If includeDrafts or a token isn't set, then the same URL is used for an entire dataset and apiVersion.
For example in Next.js apps it would let you render client components that might call client.live.events() multiple times on the same page, and still end up with a single Event Source connection. Free from the burden of implementing this dedupe logic in userland ❤

Not opposed to it, but it's also something we could provide as an operator or small wrapper util. It feels like request deduping is something that belongs more on the application/SDK side though

it's more like instance deduping. the client is already scoped to the project and dataset, so the SSE resource feels like something that should be scoped as well.
Both the Listen API and the Live API have limits to how concurrent things are (starts at 1k): https://arc.net/l/quote/edfhjttg

Listen has been difficult to use, but even then we've run into issues where you reach too many editors working in a studio.
With the Live API being so easy, this will grow. Both for production use cases, but for plugins it's become my preferred way of building things where I want to write to a document and subscribe to changes in a very lightweight way that lives outside of a form builder state.
If you install 10 plugins and they all use this technique, but we don't dedupe, you'll have easily 10 connections per user.
And it feels odd to add a wrapper like this in useClient in sanity, and then in next-sanity. hydrogen-sanity will also need it. And probably @sanity/astro. And what about @nuxtjs/sanity. 🤔

Fair and convincing points. For debuggability it might be worth considering having an option/env var to turn the caching behavior off.

bjoerge commented Jan 3, 2025

View reviewed changes

bjoerge force-pushed the refactor-eventsource branch 2 times, most recently from 0b42fa5 to f0185c1 Compare January 3, 2025 14:12

bjoerge requested a review from sgulseth January 3, 2025 14:30

bjoerge added 3 commits January 3, 2025 16:42

refactor: unify sse endpoints handling

b07b8d6

this refactors our current EventSource/SSE handling and unifies it under a common implementation shared between the live content api and the listener api.

feat: export connectEventSource

00c029a

fix(tests): update listener tests to reflect new handling of disconne…

47ca499

…ct messages

bjoerge force-pushed the refactor-eventsource branch from f0185c1 to 47ca499 Compare January 3, 2025 15:53

bjoerge commented Jan 3, 2025

View reviewed changes

bjoerge requested review from rexxars and stipsan January 3, 2025 16:06

bjoerge marked this pull request as ready for review January 3, 2025 16:06

sgulseth previously approved these changes Jan 8, 2025

View reviewed changes

stipsan previously approved these changes Jan 8, 2025

View reviewed changes

stipsan reviewed Jan 8, 2025

View reviewed changes

bjoerge dismissed stale reviews from stipsan and sgulseth via 2b4c7c7 January 8, 2025 16:26

fix: add OpenEvent to ListenEvent union

ee047bb

bjoerge force-pushed the refactor-eventsource branch from 2b4c7c7 to ee047bb Compare January 8, 2025 16:27

sgulseth approved these changes Jan 10, 2025

View reviewed changes

bjoerge added this pull request to the merge queue Jan 13, 2025

Merged via the queue into main with commit ddfd244 Jan 13, 2025
13 checks passed

bjoerge deleted the refactor-eventsource branch January 13, 2025 15:20

github-actions bot locked as resolved and limited conversation to collaborators Jan 21, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: unify sse endpoints handling #950

refactor: unify sse endpoints handling #950

bjoerge commented Jan 3, 2025 •

edited

Loading

bjoerge Jan 3, 2025 •

edited

Loading

bjoerge Jan 3, 2025

stipsan Jan 8, 2025

bjoerge Jan 3, 2025

bjoerge Jan 3, 2025

sgulseth left a comment

sgulseth Jan 8, 2025

bjoerge Jan 8, 2025

sgulseth Jan 8, 2025

bjoerge Jan 8, 2025 •

edited

Loading

stipsan left a comment

stipsan Jan 8, 2025

bjoerge Jan 8, 2025

stipsan Jan 10, 2025

bjoerge Jan 13, 2025

	export function connectEventSource<EventName extends string>(
	const EventTypeName = ListenEvent['type']
	export function connectEventSource<EventName extends EventTypeName[]>(

refactor: unify sse endpoints handling #950

refactor: unify sse endpoints handling #950

Conversation

bjoerge commented Jan 3, 2025 • edited Loading

Backwards incompatible change with regards to disconnect events

bjoerge Jan 3, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sgulseth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoerge Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

stipsan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjoerge commented Jan 3, 2025 •

edited

Loading

Backwards incompatible change with regards to `disconnect` events

bjoerge Jan 3, 2025 •

edited

Loading

bjoerge Jan 8, 2025 •

edited

Loading