undici icon indicating copy to clipboard operation
undici copied to clipboard

feat: EventSource can be configured with reconnectionTime

Open Uzlopak opened this issue 10 months ago • 10 comments

Tests can be implemented if #4247 is merged

Uzlopak avatar Jun 04 '25 00:06 Uzlopak

The dispatch option is also not spec compliant

Uzlopak avatar Jun 04 '25 05:06 Uzlopak

I don't think we should add these kind of features.

mcollina avatar Jun 05 '25 14:06 mcollina

In smee-client we use the eventsource package. And with that we patch directly the reconnectionTime to 0 for immediate reconnect. I cant set the reconnectionTime with undici. Any suggestions?

Uzlopak avatar Jun 05 '25 14:06 Uzlopak

After reading through https://github.com/whatwg/html/issues/2177, I am a +1 on adding this in a different capacity. Like WebSocket, browsers (mostly Chrome) have seemingly given up on EventSource and have held back improvements on it.

I recommend the option being put under an undici/node object, something that cannot be mistaken as anything other than a deviation from the spec. Then we could add in a dispatcher/headers object later if needed.

This also would needs docs and tests.

new EventSource('https://my.event.source/', { node: { reconnectionTime: 0 } })

KhafraDev avatar Jun 07 '25 17:06 KhafraDev

The dispatch option is also not spec compliant

I consider adding this to fetch/WebSocket a mistake. It should have been namespaced, like Cloudflare does with custom options. https://developers.cloudflare.com/workers/runtime-apis/request/#the-cf-property-requestinitcfproperties

KhafraDev avatar Jun 07 '25 18:06 KhafraDev

We could add the namespaced one and deprecate the previous one in the next release (maybe major?).

mcollina avatar Jun 14 '25 20:06 mcollina

That sounds great to me. I think it drastically reduces confusion and allows us to add whatever we want in the future, without having to regard conflicts with upcoming spec additions or other environments (ie. Deno has a "client" option which functions similarly to our "dispatcher" option, yet neither are compatible). WebSocket is a slightly different issue as passing a plain object to the constructor is a deviation from the spec in and of itself already.

We could recommend the following, which would function in all environments, but is an objectively terrible solution.

const options = ['protocols']
options.headers = { 'Authorization': 'Bearer 123456' }
options.dispatcher = ...

// with the new namespaced options:
options.node = {
  headers: { 'Authorization': 'Bearer 123456' },
  dispatcher: ...
}

const ws = new WebSocket('ws://localhost:1', options)

Currently we allow

const ws = new WebSocket('ws://localhost:1', {
  protocols: ['protocols'],
  headers:  { 'Authorization': 'Bearer 123456' },
  dispatcher: ...
})

but I think I am getting off topic now, so I digress. I spared you my rant about Chrome holding up the websocket spec, you're welcome. 😂

KhafraDev avatar Jun 15 '25 01:06 KhafraDev

@mcollina @KhafraDev

Is this to your liking? :)

Uzlopak avatar Jun 26 '25 10:06 Uzlopak

Yes but there seem to be related test failures

KhafraDev avatar Jun 26 '25 16:06 KhafraDev

@KhafraDev

Now runs through.

Uzlopak avatar Jun 30 '25 09:06 Uzlopak

@mcollina PTAL @KhafraDev PTAL

Uzlopak avatar Jul 17 '25 22:07 Uzlopak

@mcollina @KhafraDev

PTAL. I think the test is now reasonable.

Uzlopak avatar Aug 11 '25 11:08 Uzlopak