Send initialization data in the NewReport message #2317
Conversation
| fileName: "", | ||
| recording: false, | ||
| recordAudio: false, | ||
| countdown: -1, | ||
| startAnimation: false, | ||
| showRecordedDialog: false, | ||
| showScreencastDialog: false, | ||
| showUnsupportedDialog: false, |
There was a problem hiding this comment.
Unrelated to this PR, but I was making changes to App.tsx - none of these props exist
* develop: Note about using recent versions of Streamlit for sharing (streamlit#2326) Bump sphinx from 3.2.1 to 3.3.0 in /lib (streamlit#2320) Some grammatical changes in Readme (streamlit#2267) exclude v50 (streamlit#2307)
This is totally legit - it happens when the Streamlit server is killed and restarted, and the client reconnects. Test SessionInfo.fromInitializeMessage
| /** | ||
| * Handler for ForwardMsg.initialize messages | ||
| * @param initializeMsg an Initialize protobuf | ||
| */ | ||
| handleInitialize = (initializeMsg: Initialize): void => { | ||
| const { |
There was a problem hiding this comment.
This logic now lives in the "handleOneTimeInitialization" function
| !config || | ||
| !sessionState | ||
| ) { | ||
| throw new Error("InitializeMsg is missing a required field") |
There was a problem hiding this comment.
I'm assuming this part isn't necessary now. Are we just assuming the correct message is sent down (totally fine, but just checking)?
There was a problem hiding this comment.
All these checks happen within the SessionInfo constructor, so this was just essentially duplicate logic
| @@ -530,9 +471,24 @@ export class App extends PureComponent<Props, State> { | |||
| * @param newReportProto a NewReport protobuf | |||
| */ | |||
| handleNewReport = (newReportProto: NewReport): void => { | |||
| const initialize = newReportProto.initialize as Initialize | |||
There was a problem hiding this comment.
Just out of curiosity, does the protobuf not return the correct type here?
There was a problem hiding this comment.
this is the larger protobufjs typescript issue: message fields are typed as interfaces, not classes (IInitialize instead of Initialize), and the interface typings claim that fields can be undefined - when in fact, for a deserialized protobuf, there's no such thing as an undefined field (because anything not explicitly defined gets assigned a default value). So the cast here prevents us from doing a whole lot of non-null assertions on the message fields.
Removes
ForwardMsg.initialize. All initialization data is now carried in everyForwardMsg.new_reportmessage.This lets us remove the state from the server that stores whether a given client has already received the initialize message. This flag had a race condition - it was possible for the flag to be set to true, but the client not to actually have received the message, if a script used the
st.experimental_rerunfunction towards the top of a script.report_session. _maybe_enqueue_initialize_message(and its associated flag) is gone.App.handleNewReportnow conditionally calls through to a newApp.handleOneTimeInitializationfunction for the first new report message. There's logic that detects and errors if the initialize data unexpectedly changes from message to message.App.test.tsxtests that make sure initialization/newReport handling is correct.Fixes #2226