New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add st.experimental_(get|set)_query_params capability #1169
Conversation
Hey @zhaoooyue , just (belatedly) following up from our meeting a few days ago: we'll have a meeting on our end about this proposal and will get back to you. Thanks for sending the PR! |
@tvst Thanks Thiago! let me know if any further changes are needed, i.e. tests. |
Maybe |
Hey streamlit Team, thanks for this marvelous application - helps so much. Code:
|
Hey Karl and @zhaoooyue: We hope this email finds you well and safe during this unprecedented time! How we're thinking about experimental features in generalWe appreciate your innovative proposal and it prompted a lot of discussion! 🙏🏻 We want to make sure that we're responsive to the needs and efforts of the community. We are currently planning to release a new Specific thoughts on this proposalIn thinking about this proposal our goals were to:
Specific recommendations for this PRTo combine these goals we kindly and respectfully ask that you:
This will also allow us to solicit more feedback from the community about this proposal. We apologize for the delay getting back to you on this. We greatly appreciate Oak North's support for Streamlit! ❤️ |
568766f
to
6346b05
Compare
Hi @orieger - yes this behaviour is expected as the As you can see this is currently an experimental feature, and the design behind this is to not block other routes to be introduced by future Streamlit features. This is also the reason why the current design is not causing a redirect in the browser. However I certainly feel the need of your requirement - similar to @tvst mentioned earlier
These are two different routes to handle this new capability - let's wait Streamlit team's reply on this on a possible design which is also in line with future features 😄 |
@treuille Thanks for looking at the PR! As requested, I have move the two new APIs to You were also mentioning that there is somewhat conflicts between this PR and the team's Plan for State . Appreciate it if more info/feedback can be provided such I can incorporate this into the Not last - thanks for this amazing tool, as always 💯 |
Hey, apologies for the delay in reviewing! Will look at this today. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR! I left a few requests for changes, but the main issues are:
-
I think we should probably limit this feature to setting/getting only the query string. Would the code with this change still be useful to you? The rationale:
- The query string is most likely the thing the user is interested in anyway. And if we allowed getting/setting the whole URL users would have to do some URL-parsing acrobatics to get to it.
- It doesn't make sense to "set" the hostname or protocol, since those don't change for a given app.
- If you could read the host, protocol and base path it would be very easy to accidentally write an app that only works when run in a certain setup, but breaks as soon as you move it elsewhere.
And if we go this route, we can change
get_url
/set_url
to return/accept dicts instead of strings. So the user's life would be much easier 😃I added some comments about this throughout the review.
-
The code in
get_url
/set_url
is currently fairly prototype-quality... but that's OK for now, since this is experimental code. For this very reason, though, I'd rather not modify other parts of the app, likeReportContext
, unless we come up with a cleaner architecture. So it's preferable to simply monkey-patch the context inset_url
, for example.For the same reason, whatever other parts of the code must be modified (because there's no way around it), we should mark them with a comment saying "This is used in st.experimenta;.get_url".
I added some comments about this throughout the review, too.
-
Please write tests 😉
frontend/src/App.tsx
Outdated
* @param newUrl string | ||
*/ | ||
handleNewUrlChange = (newUrl: string): void => { | ||
window.history.pushState({}, "", newUrl ? newUrl : "/") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's likely a bug here: the URL should be prefixed with the basePath
from lib.UriUtil.getWindowBaseUriParts()
, since people can host apps under non-root paths too.
And following up on item 1 from the root comment in this PR, when you change the code to only allow setting the query string, then the protocol
, host
, and basePath
should all come from getWindowBaseUriParts
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this has been changed to append queryString using window.history.pushState
such the browser will not be redirected or refreshed.
this will only be called upon user calls st.experimental.set_query_string(some_value)
. on server side I have added validation to ensure frontend will only receive valid query strings, so we should be good here. Proof: Click me
thanks @tvst ! I will update the PR to address your comments accordingly |
11d1ea9
to
5e90067
Compare
Hey @zhaoooyue , LMK when you need another review. |
Hi @tvst - thanks for the thorough review lat time! I have updated the PR addressing your comments (except
I have also updated the PR description with this changes as well as a demo on how to bookmark an Streamlit app with query strings (tag #302 and this). @tvst Another review will be greatly appreciated! I will read about Streamlit unit/e2e testing approaches and add tests meanwhile. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few questions in this review. Also, there are no tests. Please add tests. At the very least, I would suggest adding unit tests. I will be waiting for the tests to be added.
A high level concern is that we are keeping the query string as a piece of state attached to the session. This can be a problem since this is a piece of info stored in the server and in the browser and it could go out of sync. Any comments? Also not a big fan of the retry loop in the get_query_string
. I haven't been part of the architectural discussion for this, but can you let me know if this has been considered? I wonder there is a way of doing this that does not require storing the query string in the session on the server.
Can someone point me to a session state implementation that's compatible with the latest nightly release? (0.64.1.dev20200731 as of now) I tried this one and got this error:
I also tried this one and got this error:
|
They have been renamed to |
Thanks @vinzeebreak! I'll give that a shot. Update: making those changes works. thanks! |
Was this feature removed again in the latest nightly? Also .experimental_set_query_params(dict) wouldnt allow to take parameters... This happend with dev20200801 |
Is it supposed to take a dict as an argument? For me passing **dict instead of dict works |
I think something broke with the latest nightly because the entire feature is gone. |
@benlindsay or @jaystary can you split this off into a new issue? I want to make sure this doesn't get lost if people aren't checking a closed PR |
I'm not sure it should be a new issue. I just tried with the dev20200801 and the feature is still there, and streamlit.server.server did not revert. @jaystary, my guess is that you installed the nightly version in one environment, then ran streamlit from another environment, which is a super easy mistake to make. I did something similar within the past week or two. Maybe run |
Looks like using @tvst's gist for SessionState will resolve the funny behavior. Here's an updated demo:
|
This is awesome!! Thanks so much @zhaoooyue @tvst @karriebear 👏 👏 👏 |
Hi All, thanks for this incredible library! |
@WilberDelbrison looking through the PR quickly, it mentions using a client state so it should be based on per user session. If this is still an issue, could you please open a new issue with a reproducible code sample for us to look at? |
can You pls help me do this how to do it how to install am very new to streamlit with clear steps it would be more helpful |
@vini-2907 there's no need to install anything else. Anything under the experimental namespace are available in streamlit just prefaced with You can learn more about our experimental namespace in our docs |
Works for me except the order of two lines has to be changed like so: from this:
to that:
And for completeness' sake: for the SessionState I had to use https://gist.github.com/FranzDiebold/898396a6be785d9b5ca6f3706ef9b0bc |
@karriebear thanks for the explanation (above) but I don't understand why the default from the URL parameters is being used once the dashboard has already loaded. The API docs suggest that the default
Shouldn't all widgets always show and return any value that comes from the browser, and only use the given default if none? |
Hi Everyone, Thanks for adding these powerful capabilities to Streamlit! Building on your work, I made a library of url-aware versions of input controls that automatically sync their state with the url query string: streamlit-permalink. Hopefully some day these ideas will find their way into Streamlit core and such external libraries would no longer be needed :) |
Issue: Issue #798 #302 #1098 This PR is proposing a simple change to add capability to set and get current query strings from browser URL in user's browser tab (without triggering a navigation event!). By introducing this feature to
st.experimental
namespace, a Streamlit user can easily bookmark an application using query strings and reopen the application else where and render all states using the query strings!Demo
Feature:
1️⃣ User can call
st.experimental_set_query_params
=> to set query params for current browser tabExample
st.experimental_set_query_params( checkbox1=True, multiselect2=2 )
, the user’s browser tab will become http://localhost:3000/?checkbox1=true&multiselect2=2 and this will NOT cause a redirect action.2️⃣ User can call
st.experimental_get_query_params
=> _to get the current query strings as a dict {"checkbox1": "true", "multiselect2=2"}Example Usage from above demo
Contribution License Agreement
By submiting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.