Have theme hashes differentiate between being unset and the hash for null #2993
Conversation
tconkling
left a comment
tconkling
left a comment
There was a problem hiding this comment.
Is there a simple test that fails on the current behavior and passes with this change?
frontend/src/App.tsx
Outdated
| this.handleSessionStateChanged(initialize.sessionState) | ||
| } | ||
|
|
||
| createThemeHash = (themeInput: CustomThemeConfig): string | null => { |
There was a problem hiding this comment.
Is null still a valid return value from this function?
There was a problem hiding this comment.
Nope, changed the return value type to just be string here
frontend/src/App.tsx
Outdated
| // Differentiate between the case where this.state.themeHash has yet to | ||
| // be computed (so this.state.themeHash === null) and the case where we | ||
| // received no custom theme from the server. |
There was a problem hiding this comment.
This comment and function is confusing to me. I might just be lacking context, so take this with a grain of salt, but here's how (I think) this could be made clearer:
- Change the function's return value to
: string. That makes it clear that a non-null, non-undefined string must be returned. (I think this is already the case?) - Change
this.state.themeHash's type tothemeHash?: string. An undefined themeHash has a clear meaning: it's simply not defined yet.this.state.themeHashwill never be null. - Change this comment to say something like: "If themeInput is null, then the app has no custom theme. We use a hardcoded string literal for the hash in this case."
- Nit: "no_theme_input" isn't particularly descriptive. What about "undefined_theme_hash" as a string, instead?
There was a problem hiding this comment.
Thanks! I agree that changing the type of this.state.themeHash to be optional (and making changes for your other suggestions) does make things more clear here.
vdonato
left a comment
vdonato
left a comment
There was a problem hiding this comment.
Is there a simple test that fails on the current behavior and passes with this change?
The test on App.test.tsx line 389 serves that purpose after the line setting themeHash to "hash_for_undefined_custom_theme" was added.
Then the tests where the wrapper.setState({ themeHash: "someHash" }) lines were removed cover the cases where a custom theme option is being removed but we haven't processed its removal yet
frontend/src/App.tsx
Outdated
| this.handleSessionStateChanged(initialize.sessionState) | ||
| } | ||
|
|
||
| createThemeHash = (themeInput: CustomThemeConfig): string | null => { |
There was a problem hiding this comment.
Nope, changed the return value type to just be string here
frontend/src/App.tsx
Outdated
| // Differentiate between the case where this.state.themeHash has yet to | ||
| // be computed (so this.state.themeHash === null) and the case where we | ||
| // received no custom theme from the server. |
There was a problem hiding this comment.
Thanks! I agree that changing the type of this.state.themeHash to be optional (and making changes for your other suggestions) does make things more clear here.
# By Vincent Donato (3) and Nikitas Rontsis (1) # Via GitHub * develop: Have theme hashes differentiate between being unset and the hash for null (#2993) Improve module reloading (#3001) Add tests to improve coverage around config options set via flag (#2991) Add snippets about CSS vars and minimum streamlit-component-lib versions (#3008) # Conflicts: # frontend/src/App.tsx
2 participants
There's a bug that I introduced when adding the theme hashing code that can be
reproduced in two ways.
Way 1 (which is really easy to run into.. not sure how this slipped by 😞):
the theme name in the theme selector is empty.
Way 2:
theme name in the theme selector is empty.
The root cause of both of these bugs is that we're currently not differentiating
between the theme hash being unset because the app just loaded and it being
a theme hash corresponding to a null theme input. This means when a browser
first loads the app and any custom theme should be removed (because we don't
get one from the server), we don't actually do so.