Allow the parent frame of a streamlit app to terminate/restart its websocket #8704
Conversation
vdonato
force-pushed
the
vdonato/host-websocket-msgs
branch
3 times, most recently
from
c657a18
to
96971e0
Compare
| .iframe(() => { | ||
| cy.waitForScriptFinish() | ||
| }); | ||
| before(() => { |
There was a problem hiding this comment.
Some things to note:
- The vast majority of the changes to this file were applied by the autoformatter. Not sure how this file
ended up with 4-space indents instead of 2. - The only actual change made to this file is the test added to the very end
- We should be migrating this test to playwright soon, but it only took ~5 minutes to add a test here
vs probably the day+ of work it'll take to migrate this test, so I think it's still worth doing this now
to get the PR merged sooner than later. I added a TODO for myself to go back and migrate things later.
There was a problem hiding this comment.
I actually had this same issue as well. I think we should migrate this to playwright, but I was going to suggest after MPA v2.
There was a problem hiding this comment.
@raethlein added a new playwright fixture to run an app in iframe recently. The hostframe logic could potentially be integrated into this iframe fixture.
There was a problem hiding this comment.
here is an example test using the new fixture: https://github.com/streamlit/streamlit/blob/develop/e2e_playwright/st_dataframe_interactions_test.py#L329 in case it helps 🙂
vdonato
force-pushed
the
vdonato/host-websocket-msgs
branch
2 times, most recently
from
72a5d25
to
b43e734
Compare
vdonato
changed the title
|
Heads up @sfc-gh-pfinnigan @sfc-gh-shaar that these message types should be ready for e2e testing in SiS. Note the names of the messages are currently:
but I'm not very opinionated on these names and am happy to change them if any better one are suggested. Also, I haven't started looking into websocket close codes or backoffs on ws reconnect retries just yet, but I'll be doing some investigation of those over the next few days. |
vdonato
force-pushed
the
vdonato/host-websocket-msgs
branch
from
b43e734
to
727dfc9
Compare
|
Temporarily adding the |
| .iframe(() => { | ||
| cy.waitForScriptFinish() | ||
| }); | ||
| before(() => { |
There was a problem hiding this comment.
I actually had this same issue as well. I think we should migrate this to playwright, but I was going to suggest after MPA v2.
| this.hostCommunicationMgr.sendMessageToHost({ | ||
| type: "SCRIPT_RUN_STATE_CHANGED", | ||
| scriptRunState: this.state.scriptRunState, | ||
| }) |
There was a problem hiding this comment.
What's the reasoning for the Script Run state changed event here? Is it just for the test?
I offered an idea that might make more sense, but in any case a comment here would be helpful to relate the two concepts if they are related.
|
|
||
| // This test isn't ideal, but since we can't inspect the state of our app | ||
| // using RTL, we have to check that some other side effect of calling | ||
| // initializeConnectionManager occurs. |
There was a problem hiding this comment.
I know it sounds silly, but have we considered also have a WEBSOCKET_CONNECTING message? No one needs to know it, but it does handle this in between state well.
We want to be able to let the parent frame of a streamlit app manage its websocket connection
lifecycle. This PR adds this functionality to our
HostCommunicationManagerclass by adding fournew message types for interacting with a Streamlit app's websocket connection:
RESTART_WEBSOCKET_CONNECTIONTERMINATE_WEBSOCKET_CONNECTIONWEBSOCKET_DISCONNECTED(along with an attemptingToReconnect flag in the message payload)WEBSOCKET_CONNECTED