feat: EventSource can be configured with reconnectionTime #4260
Conversation
|
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 |
|
After reading through whatwg/html#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 This also would needs docs and tests. new EventSource('https://my.event.source/', { node: { reconnectionTime: 0 } }) |
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. 😂 |
…cate dispatcher attribute on top level
Uzlopak
force-pushed
the
reconnection-time
branch
from
11d3380 to
0254706
Compare
|
Is this to your liking? :) |
|
Yes but there seem to be related test failures |
|
Now runs through. |
| onerrorCalls.push(error) | ||
| } | ||
|
|
||
| await new Promise(resolve => setTimeout(resolve, 5000)) |
There was a problem hiding this comment.
This test looks like it will be very flaky
There was a problem hiding this comment.
We already have one test similar to this in the codebase for weeks and it was not very flaky in the ci. Or was it?
There was a problem hiding this comment.
I don't understand that test - it gives 8 seconds for 3 connection errors despite the default reconnection time being 3 seconds? I would expect anything that relies on timing to be flaky, especially when the timing is strict, such as with this test. You give exactly 5 seconds for 5 reconnection errors with a 1 second reconnection time.
|
@mcollina PTAL |
Tests can be implemented if #4247 is merged