-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Rename endpoints #5534
Rename endpoints #5534
Conversation
Just had a quick look through some things from product side and looks great! Make sure we also update the snippets in the docs that are affected by this. Talk to @snehankekre if you need help, he's owning our docs (might still be OOO though). |
8f78f3d
to
d439393
Compare
d439393
to
e66e9b7
Compare
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.
Hey @sfc-gh-kbregula,
We'll want to do this eventually, but I think we may want to hold off for a few months to coordinate with some other teams. I'll follow up in Slack with more details.
For now, I'm requesting changes to ensure this isn't merged, and I think what we'll want to do is close this PR but keep the branch alive until ~January or so, at which point we should be able to make these changes.
Thankfully, these areas of the code are rarely changed, so I don't think leaving the branch alive until then will be too expensive.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
@sfc-gh-kbregula heads up that, given the plans that the SiS team currently has (which is to not update their Streamlit lib version until after PrPr launches), we may be able to go ahead and get this change merged into I'll start a conversation in Slack next week since I think most people on other teams that would have an opinion on this are out this week for the holidays. |
d9d229d
to
f2f8a40
Compare
@vdonato I update the PR. I deleted all deprecation warnings for endpoints that are internal. Now we have the deprecation warnings for the following endpoints:
|
90666cf
to
bafb6b8
Compare
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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.
Sorry for the delay on reviewing this @sfc-gh-kbregula!
I have two very minor comments, but otherwise this LGTM.
I'm going to leave this in the "Changes requested" state for now because we still need to finalize the exact process we'll be following for making changes that have implications for the SiS team (I was a bit ambitious to think that we could figure this out early), but once that's sorted out, this PR should be ready to be merged.
I hope to get the process for making this change finalized early next year, but I think the only necessary thing to do will be to document the necessary changes that a service replacing the Tornado server will have to implement to be compatible with the Streamlit web client (in this case, just renaming a few endpoints to the new _stcore/*
versions)
lib/streamlit/web/server/server.py
Outdated
@@ -77,8 +77,16 @@ | |||
# to an unix socket. | |||
UNIX_SOCKET_PREFIX = "unix://" | |||
|
|||
# The endpoint we serve media files from. | |||
# The endpoints |
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.
nit: I think it'd be fine to just remove this comment entirely
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.
Removed.
lib/streamlit/web/server/routes.py
Outdated
if config.get_option("server.enableXsrfProtection"): | ||
self.set_cookie("_xsrf", self.xsrf_token) |
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.
nit: I think it should be okay to remove this and allow the /_stcore/health
endpoint be reponsible for setting the browser tab's xsrf
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.
Removed.
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. |
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.
@sfc-gh-kbregula sorry that it took awhile to confirm that we should be able to proceed with this change -- we should be unblocked on being able to merge this PR now!
8b9dd65
to
4a484d5
Compare
All fixed. When CI passes, I will merge these changes tomorrow. |
We waiting for QA team. |
bb17170
to
5dcf626
Compare
QA done. Now I'm doing a rebase and merging the change. |
* Update the streamlit submodule to v1.18.1 * Update the wheel file name * Update react-scripts to 5.0.1 * Replace path with path-browserify in the worker, that were automatically polyfilled by Webpack4 but it isnt' with v5 * Remove worker-loader that has been deprecated for Webpack5 * Update dependencies and craco config in @stlite/sharing adapting to streamlit@1.18.1 * Update dependencies and craco config in @stlite/desktop adapting to streamlit@1.18.1 * Set streamlit-browser dependency version * [WIP] Override mountable/* with files ejected from a newly scaffolded package with CRA5.0.1 * Customize the build config and dependency list for @stlite/mountable upon the ejected files * Update dependency versions following the streamlit-browser package * Fix typing in sharing-editor * Update mountable/DEVELOPMENT.md * Update yarn.lock * Rename the websocket endpoint path following streamlit/streamlit#5534 * Fix packages/kernel/py/e2etests/tests/test_httpserver.py * Fix the CI to run test-tornado-e2e anytime * Fix tornado-e2e tests * Fix CI * Replace @st.experimental_memo with @st.cache_data in the samples * Rename the file upload endpoint path following streamlit/streamlit#5534 * Fix BROWSER envvar value for CRA5 * Fix to avoid forbidden access to LocalStorage due to #476 * Add streamlit-browser@^1.18.1 to the deps list of @stlite/mountable * Fix comments in packages/mountable/config/webpack.config.js * Remove the type declarations for inline raw-loader import * Move the deps list from dependencies to devDependencies in @stlite/mountable * Introduce a hack to handle cross-origin policy of worker creation * Set local package name for dump_artifacts.ts * Fix CSP
* Update the streamlit submodule to v1.18.1 * Update the wheel file name * Update react-scripts to 5.0.1 * Replace path with path-browserify in the worker, that were automatically polyfilled by Webpack4 but it isnt' with v5 * Remove worker-loader that has been deprecated for Webpack5 * Update dependencies and craco config in @stlite/sharing adapting to streamlit@1.18.1 * Update dependencies and craco config in @stlite/desktop adapting to streamlit@1.18.1 * Set streamlit-browser dependency version * [WIP] Override mountable/* with files ejected from a newly scaffolded package with CRA5.0.1 * Customize the build config and dependency list for @stlite/mountable upon the ejected files * Update dependency versions following the streamlit-browser package * Fix typing in sharing-editor * Update mountable/DEVELOPMENT.md * Update yarn.lock * Rename the websocket endpoint path following streamlit/streamlit#5534 * Fix packages/kernel/py/e2etests/tests/test_httpserver.py * Fix the CI to run test-tornado-e2e anytime * Fix tornado-e2e tests * Fix CI * Replace @st.experimental_memo with @st.cache_data in the samples * Rename the file upload endpoint path following streamlit/streamlit#5534 * Fix BROWSER envvar value for CRA5 * Fix to avoid forbidden access to LocalStorage due to whitphx/stlite#476 * Add streamlit-browser@^1.18.1 to the deps list of @stlite/mountable * Fix comments in packages/mountable/config/webpack.config.js * Remove the type declarations for inline raw-loader import * Move the deps list from dependencies to devDependencies in @stlite/mountable * Introduce a hack to handle cross-origin policy of worker creation * Set local package name for dump_artifacts.ts * Fix CSP
📚 Context
Close: #3028
Spec: https://www.notion.so/streamlit/Product-spec-1caef53a784d4482a129aca184c000a5
Please describe the project or issue background here
What kind of change does this PR introduce?
🧠 Description of Changes
Add bullet points summarizing your changes here
Revised:
Insert screenshot of your updated UI/code here
Current:
Insert screenshot of existing UI/code here
🧪 Testing Done
🌐 References
Does this depend on other work, documents, or tickets?
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.