Support `onCleanup` callback to schedule side effect cleanup when resolvers are finished

[piotr-cz]

Scope

Improves an existing behavior

Compatibility

• This is a breaking change

Feature description

Events pushed with client.send are being logged in DevTools console even when the request is terminated/ finished (either by ServerSentEventClient or an app)

Example

I'm using client.send method to emit ping events in a fixed interval.

I'd like to test connection recovery in case of network issues either by client.close() or client.error().

When connection is terminated using one of above methods, my app opens new connection.
In DevTools console I can see new ping events reported for new and each terminated connection.

Proposed solutions

There could be a way to notify SSE handler that connection has been terminated, so it may stop sending events or perform any other cleanups.

import { sse } from 'msw'

const handler = sse('/stream', ({ client, request }) => {
  // Schedule ping
  const pingTimer = setInterval(() => client.send({ event: 'ping', data: null }), 60_000)

  const cleanup = () => clearInterval(pingTimer)

  // Cleanup using request.signal
  request.signal.addEventListener('abort', cleanup)

  // Cleanup using client.onabort
  client.onabort = () => cleanup

  // Cleanup using returned function
  return cleanup
})

[kettanaito]

Hi, @piotr-cz. Thanks for suggesting this. It does look like a bug on our end. Do you happen to have a reproduction scenario for this? Would make for a great test.

[kettanaito]

I would not suggest implementing this via request.signal. That signal stands exclusively for "this request has been aborted". The outlined issue is that there's no hook to subscribe to when the underlying HTTP request for SSE is no longer used. That might happen outside of it being aborted:

• The client closes the connection;
• The mock closes the connection;
• The original server closes the connection and it's forwarded to the mock -> client.

We shouldn't interfere with request.signal and alter its purpose. This probably calls for a proprietary API.

[kettanaito]

Can this use case be achieved via disposable objects?

sse('/target', () => {
  const interval = setInterval(fn, 1000)

  using _ = {
    [Symbol.dispose]() {
      clearInterval(interval)
    }
  }
})

If a strict timing isn't a requirement, the handler's closure will get garbage-collected eventually. I'm just worried that "eventually" might come really late and is up to the V8 judgment.

[kettanaito]

I've got two ideas right now, disregarding the naming:

// 1: Callback.
sse('/stream', ({ client, onFinished }) => {
  const interval = setInterval(effect, 1000)
  onFinished(() => clearInterval(interval))
})

// ✅ Clear and explicit
// ❌ Granular and doesn't scale well

// 2: EventTarget.
sse('/stream', ({ client, target }) => {
  const interval = setInterval(effect, 1000)
  
  target.addEventListener('complete', () => clearInterval(interval))
})

// ✅ Scales well with different event types
// ✅ Based on standard APIs
// ❌ EventTarget is terrible to type-annotate
// so the API will likely be non-standard anyway.
// ❌ EventTarget is open for `.dispatchEvent()`
// and isn't a readonly API (+1 for non-standard approach).

[piotr-cz]

Using disposable object doesn't work here, because [Symbol.dispose]() is called when engine reaches end of callback function.

The Callback option seems simpler to me than using EventTarget.

I still like the my option to return a callback from a handler, just like in React's useEffect.

[kettanaito]

I still like the my option to return a callback from a handler, just like in React's useEffect.

I understand the appeal but that is unlikely to happen. I am particularly strict when it comes to what handlers can do and are responsible of. A handler only returns a mocked response (or an instruction about it), nothing else. It's hard to overstate how important that limitation has proven over the years. We would have to find a way to provision the cleanup through the resolver arguments or even through some API that lives outside of a request handler.

I am quite fond of the Disposable pattern that we use throughout the library. Maybe something like this would help:

http.get('/resource', ({ subscriptions }) => {
  const interval = setInterval(fn)
  subscriptions.push(() => clearInterval(interval))
})

This is a really flexible way of scheduling a cleanup while delegating when that happens to the library's context. But at this point, it's basically onFinished with extra steps.

[piotr-cz]

okay, I get it.

In that case using the onFinished callback looks best to me.

[piotr-cz]

I came up with one more idea: passing an AbortSignal in handler argument, like your with EventTarget:

from 'msw'

const handler = sse('/stream', ({ client, signal }) => {
  // Schedule ping
  const pingTimer = setInterval(() => client.send({ event: 'ping', data: null }), 60_000)

  // Cancel ping on abort
  signal.addEventListener('abort', () => clearInterval(pingTimer))
}

[kettanaito]

The AbortSignal, while extending EventTarget, has a semantic connotation of listening to when something gets aborted. I find it a poor candidate to use to listen to when the handler is done running. If you like its ergonomics, I suggest a simple EventTarget instead.

[kettanaito]

Proposal: finalize()

I am proposing to add a new function to the response resolver object argument called finalize(). It accepts a callback that is either sync or async, and never returns a promise itself so the developer isn't tempted to await it for no reason.

The function executes the given callback LIFO after the handler is completely done but before the mocked response is actually sent, if any.

server.use(
  http.get('http://localhost/resource', ({ finalize }) => {
    finalize(cleanup)
  }),
)

The function will be present on all handlers derived from RequestHandler, which means http, graphql, and sse. For WebSockets, such a callback is unneeded as one can simply listen to the connection closure on the client to achieve the same result.