-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
Update custom-components import paths and tests #8666
base: develop
Are you sure you want to change the base?
Conversation
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.
Code changes LGTM, but I'm guessing whether or not we want to revive this behavior (given it was an unintentional implication of how the Python module system works) ends up being on the product team to make a final decision.
If not many people are running into this, I'd lean toward not doing so given this was never supposed to be exported to begin with, but I don't have a strong opinion either way
fyi Vincent's opinion on this @jrieke @sfc-gh-jcarroll |
My understanding is we only saw this a couple times in the wild and it's easy to fix in a backwards compatible way inside of a given component or app. Assuming that's true, I'm also fine to NOT revive this behavior and not merge this (and I think I have a slight preference to same). We should just make sure we fix the instances like these in the docs that refer to this now-not-functioning behavior. Unless someone else sees a good reason to keep it. cc @sfc-gh-dmatthews thoughts? |
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. |
I'm not sure if I'm reading this correctly. I just tested on nightly and I can import |
@sfc-gh-dmatthews The issue is that it was possible to use the commands like import streamlit as st
st.components.v1.html(...) as suggested also in some parts of our docs, but this is not possible anymore (and was only possible coincidentally). We would like to update our docs to make it more clear that you can only import it like import streamlit.components.v1 as components
components.html(...) |
Describe your changes
Closes #8644
From this comment:
The usage of
components.html
viast.components.v1.html
was possible prior to1.34
due to this import:from streamlit.components.v1.components import ComponentRegistry
, which made it transiently available. This means that the usage viast.components.v1.html
is only possible whenserver.py
is imported at one point, which is not the case for example when running Streamlit as a bare-script.We have discussed this and thought about bringing the behavior back, but this time explicitly and with tests 🙂
Final decision is still pending.
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.