Add websocket hot reloading #478
Conversation
yissachar
force-pushed
the
yissachar/add-websocket
branch
from
cc3420f
to
7145cb3
Compare
| .url("ws://${uri.host}:${uri.port}/ws") | ||
| .build() | ||
|
|
||
| val wsFlow:Flow<String> = callbackFlow { |
There was a problem hiding this comment.
All of this code feels super wonky. I'm not a flows expert but surely there's a better way to do this?
|
|
||
| val uri = URI(manifestUrl) | ||
| val request: Request = Request.Builder() | ||
| .url("ws://${uri.host}:${uri.port}/ws") |
There was a problem hiding this comment.
I don't love this approach of hardcoding the expected websocket URL, but I'm not sure where we would expose it as a config, and it doesn't seem like a huge problem since we're not likely to change the Zipline server often.
| import okhttp3.WebSocketListener | ||
|
|
||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| internal actual fun hotReloadFlow(flow: Flow<String>): Flow<String> { |
There was a problem hiding this comment.
Would like to add some tests to validate this behaviour if I get the time.
| ws?.close(1000, null) | ||
| } | ||
| } | ||
| wsFlow.onEach { emit(it) }.collect() |
There was a problem hiding this comment.
You can delete this and switch to flattMapLatest from which you can return the inner flow and it will be automatically subscribed to.
redwood-treehouse/build.gradle
Outdated
| iosMain { | ||
| dependsOn(nativeMain) | ||
| } |
There was a problem hiding this comment.
Was this needed? The iosMain source set and its dependence on nativeMain should already have been configured globally for all multiplatform modules.
There was a problem hiding this comment.
I had to add it to resolve some error, but doesn't look like that's the case anymore - possibly because I was actually working on a really old branch, and then rebased against latest.
|
|
||
| var currentlyConnectedToWebSocket = false | ||
|
|
||
| val uri = URI(manifestUrl) |
There was a problem hiding this comment.
Jesee would probably want you to use OkHttp's HttpUrl here!
There was a problem hiding this comment.
Plus you don't actually have to use the ws scheme with OkHttp so you can do HttpUrl.get(manifestUrl).resolve("/ws") and be done.
| val wsClient: OkHttpClient = OkHttpClient.Builder() | ||
| .readTimeout(0, TimeUnit.MILLISECONDS) | ||
| .connectTimeout(1, TimeUnit.SECONDS) | ||
| .build() |
There was a problem hiding this comment.
A new client is being created for every upstream emission when what you probably want is to share this instance for the lifetime of the downstream subscription. You can do that with a level of nesting such as
return flow {
val client = OkHttpClient.Builder()...build()
emitAll(upstream.flatMapLatest { logic... })
}
|
Looks like MockWebServer doesn't have proper support for testing websockets (or at least I couldn't figure it out), so I'll just merge and try to talk to Jesse when he's back to see if there's a different way to test this. |
| while (true) { | ||
| if (!currentlyConnectedToWebSocket) { | ||
| trySendBlocking(manifestUrl) | ||
| println("we are here") |
There was a problem hiding this comment.
It was removed in a follow-up
Adds websocket hot reloading for Android. Shaves 400-500ms off of hot reload times. For now leaving iOS as 500ms polling but will follow-up with another PR once that's working.
Note that right now there is no websocket on the server. This code will fallback to 500ms polling when we can't connect to a websocket, but also tries to make a websocket connection on each poll, in case the websocket is only down temporarily (e.g. in the case of a dev leaving emulator running, stopping hot reload server, then restarting, we want to make sure we always upgrade to websocket where possible to get the fastest speeds).