-
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
Change the fragmentId
to be a required property in WidgetStateManager
#8533
Conversation
Hm, so I think it'd probably be a good idea to convert this PR into a draft for now and pick up the work again when removing the Happy to take this PR over once I start working on fragment de-experimentalization in about a month or so. |
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. |
3d89471
to
beea654
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.
Just did a quick read-through of the notebooks code importing things from @streamlit/lib
, and there doesn't seem to be any usage of any of the methods that we're changing the signatures of here.
Probably could have done this far earlier (like while initially implementing fragments so that this refactor wouldn't have been required to begin with), whoops 🙈
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.
LGTM 👍
There are also two other refactorings related to this I was thinking about that might be worth to consider / discuss as some follow ups PR:
- Move the formId into its own parmater similar to fragmentId. This is a bit of a problem with effect dependencies that we currently pass the element object as first parameter -> and the element object doesn't work well as effect dependencies since it doesn't check for deep equality only object reference equality. Splitting it up into
elementId
andformId
would make it safer to add these to effect dependencies. - Refactor the Source property (only contains fromUI prop) into a more descriptive paramter: e.g.
syncState: bool
ortriggerRerun: bool
.
E.g. the signature could look something like this:
public setStringValue(
elementId: string,
value: string | null,
formId: string | undefined,
fragmentId: string | undefined,
triggerRerun: bool = true,
)
wdyt?
Yep -- both refactors sound quite reasonable to me. As far as I can tell, all of the usage of these methods at the moment are internal to |
Describe your changes
The
fragmentId
is currently an optional property for all the set state methods in theWidgetStateManager
. The reason here is that it sometimes has a value and sometimes is justundefined
. But this makes it a bit too easy to forget to add it -> which can lead to features that are not compatible with fragments.This PR changes the property to required by changing from
fragmentId?: string
tofragmentId: string | undefined
. This also uncovered a potentially existing issue with form submission & fragments, which will be fixed via this PR, e.g.:https://github.com/streamlit/streamlit/pull/8533/files#diff-b6b2b8d7411dc40d04d4dc0442572c4835e84a79844cd2e299a469b7b2ec1a1cR309
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.