feat: EventSource can be configured with reconnectionTime
Tests can be implemented if #4247 is merged
The dispatch option is also not spec compliant
I don't think we should add these kind of features.
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?
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 } })
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
We could add the namespaced one and deprecate the previous one in the next release (maybe major?).
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. 😂
@mcollina @KhafraDev
Is this to your liking? :)
Yes but there seem to be related test failures
@KhafraDev
Now runs through.
@mcollina PTAL @KhafraDev PTAL
@mcollina @KhafraDev
PTAL. I think the test is now reasonable.