support backend hmr #277
support backend hmr #277
Conversation
| plugins: [ | ||
| react(), | ||
| // reload on backend change | ||
| { |
There was a problem hiding this comment.
Why do we need this on the client, to reload the websocket connection?
There was a problem hiding this comment.
this should just be reloading the ws on the client whenever the API rebuilds in dev mode which is needed otherwise it's connected to a nonexistent session and breaks but @swk777 correct me if i'm wrong on that
There was a problem hiding this comment.
this should just be reloading the ws on the client whenever the API rebuilds in dev mode which is needed otherwise it's connected to a nonexistent session and breaks but @swk777 correct me if i'm wrong on that
There was a problem hiding this comment.
That's what I assume. I would be ok with this as a stop-gap but the better solution here is for the client side websocket client to re-establish connections when the connection is closed. Robust websocket clients use a combination of heartbeats and onclose listeners to attempt reconnect with a backoff strategy.
For now I'm ok to merge this but I would like to comment and/or file an issue to capture this.
There was a problem hiding this comment.
There is already some retry logic inside of the client, but I think this is retrying to send a message if it fails, so maybe it doesn't handle this case properly.
https://github.com/srcbookdev/srcbook/blob/main/packages/web/src/clients/websocket/client.ts#L10-L11
There was a problem hiding this comment.
@versecafe is right, and as @benjreinhart mentioned, this approach is indeed better. I’ll work on implementing this later.
Another point that deserves attention is that in the current design, the cellId is randomly generated, which changes after a reload. As a result, cell operations cannot be continued seamlessly across hmr without reload.
There was a problem hiding this comment.
That's a good point, we need to fix those.
| @@ -9,7 +9,7 @@ | |||
| "isolatedModules": true, | |||
| "useDefineForClassFields": true, | |||
| "lib": ["es2022", "DOM", "DOM.Iterable"], | |||
| "module": "Preserve", | |||
| "module": "esnext", | |||
There was a problem hiding this comment.
Can you explain this change? cc @versecafe
There was a problem hiding this comment.
@benjreinhart there shouldn't be a huge difference between esnext and preserve but removing preserve allows for some small transformations in tsc for cleaning things up, this should be a fine change and even make things easier but does mean a test to make sure the full bundled version is resolving correctly needs to be done to prevent a mishap like the 0.0.1 release bug we had
There was a problem hiding this comment.
Sweet, so long as it's all working with no major downsides LGTM
There was a problem hiding this comment.
Thank you @versecafe for the confirmation. I've discovered that "module: preserve" doesn't work well with vite-node watch mode. If necessary, we can consider keeping "module: esnext" specifically for the development mode @benjreinhart
This reverts commit c14ceab.
No description provided.