Add connected state to turbo-cable-stream-source
#430
Conversation
| class SectionChannel < ApplicationCable::Channel | ||
| extend Turbo::Streams::Broadcasts, Turbo::Streams::StreamName | ||
| include Turbo::Streams::StreamName::ClassMethods | ||
|
|
||
| def subscribed | ||
| stream_name = [ params[:section], verified_stream_name_from_params ].join("_") | ||
| stream_from stream_name |
There was a problem hiding this comment.
This channel is used by the "passing extra parameters to channel" broadcast test. Previously it worked by echoing back a message with the parameter on subscription, so it could verify it got the correct parameter back. However broadcasting during subscription setup also has a race condition that means sometimes that message will be sent before the client is ready to receive it.
So this now takes a different approach. It uses the param as part of the stream name, and then verifies that it can stream to the expected name. This lets us structure the test the same way as the other broadcast tests, including the same mechanism for waiting on readiness.
| this.subscription = await subscribeTo(this.channel, { | ||
| received: this.dispatchMessageEvent.bind(this), | ||
| connected: this.subscriptionConnected.bind(this), | ||
| disconnected: this.subscriptionDisconnected.bind(this), |
There was a problem hiding this comment.
Not for this PR, but we might be able to use fat arrows instead of bind(this). Caveat is that public class fields are only available in Safari > 14.1.
There was a problem hiding this comment.
BTW, no extra comma at the end of the object literal.
There was a problem hiding this comment.
I don't think these are "public class fields", they are just regular methods on the class
There was a problem hiding this comment.
D'oh, thanks, it was added by prettier. We should configure a linter for this repo (but that's also not for this PR!)
There was a problem hiding this comment.
@marcoroth I mean that you can directly define an event handler bound to this within a class using a fat arrow:
class EventHandler {
doSomething = (event) => {
...
}
}But that's implicitly using the public class fields feature.
There was a problem hiding this comment.
@afcapel ah, I see what you mean, I misunderstood! Yeah, that's right 👍🏼
| @@ -4061,6 +4063,12 @@ class TurboCableStreamSourceElement extends HTMLElement { | |||
| }); | |||
| return this.dispatchEvent(event); | |||
| } | |||
| subscriptionConnected() { | |||
| this.setAttribute("connected", ""); | |||
There was a problem hiding this comment.
Would it make sense to add either add a timestamp or date to the attribute value, so that you could use that value to know when it was "[last] connected"?
| this.setAttribute("connected", ""); | |
| this.setAttribute("connected", new Date()); |
or
| this.setAttribute("connected", ""); | |
| this.setAttribute("connected", Number(new Date())); |
There was a problem hiding this comment.
I'm inclined to leave it as-is, because I prefer the simplicity of a boolean attribute here.
That said, I also can't really think of a case where the timestamp would be needed. I'm only interested in the current state. Did you have a specific case in mind where you would need it?
When a `turbo-cable-stream-source` is added to a page, it asynchronously opens a subscription to the appropriate stream. However it did not provide a way to programmatically tell if/when that subscription was connected. This makes it hard to write reliable tests that depend on streams being ready for use. Or to expose that connection state in the UI (for example, to show a warning when connection is lost). To address this, we can add a `connected` attribute to the `turbo-cable-stream-source` element whenever it is connected. Using this, we can fix the flakiness in our broadcast tests, as now they can reliably wait for a `turbo-cable-stream-source[connected]` to be on the page before attempting to broadcast anything.
kevinmcconnell
force-pushed
the
tests/broadcast-flakiness
branch
from
a577fb8
to
421884b
Compare
When a
turbo-cable-stream-sourceis added to a page, it asynchronously opens a subscription to the appropriate stream. However it did not provide a way to programmatically tell if/when that subscription was connected.This makes it hard to write reliable tests that depend on streams being ready for use. Or to expose that connection state in the UI (for example, to show a warning when connection is lost).
To address this, we can add a
connectedattribute to theturbo-cable-stream-sourceelement whenever it is connected.Using this, we can fix the flakiness in our broadcast tests, as now they can reliably wait for a
turbo-cable-stream-source[connected]to be on the page before attempting to broadcast anything.