google / mesop

Build delightful web apps quickly in Python
https://google.github.io/mesop/
Apache License 2.0
4.63k stars 214 forks source link

Component Tree Diff Edge case leading to components not being removed on update (maybe) #311

Open richard-to opened 1 month ago

richard-to commented 1 month ago

I haven't verified if this is a bug with the component tree diff or not yet. I'd have to figure out how to reproduce it first.

I had some code like this:

# Event handler on the radio. 
me.radio(...)
if state.radio_result is True
  me.text("Success")
elif state.radio_result is False:
  me.text("Fail")

What happened was every time I clicked the radio button, the me.text was appended and not replaced.

But this variation worked fine:

# Event handler on the radio. 
me.radio(...) 
me.text("Success" if state.radio_result else me.text("Fail"))
wwwillchen commented 1 month ago

I couldn't reproduce your scenario, but I was able to reproduce another issue with component tree diffs:

import mesop as me

@me.page(path="/diff_edge_case")
def app():
    me.text("diff_edge_case")
    me.radio(options=[me.RadioOption(label="1", value="1",),
                      me.RadioOption(label="2", value="2",)], on_change=on_change)
    if me.state(State).value == "1":
        me.checkbox("check me")
    else: 
        me.checkbox("i will be erroneously checked")

@me.stateclass
class State:
    value: str ="1"

def on_change(e: me.RadioChangeEvent):
    me.state(State).value = e.value

If I check the checkbox, and then I toggle the radio option, the checkbox (the second one) should be unchecked, however because I believe the diff algorithm thinks the component instance is the same, it'll treat the second checkbox as the same instance as the first checkbox and the checkbox will be erroneously checked.

You can manually fix this error by setting different key values for each checkbox and then it behaves as expected.

I think one possible way of fixing this is by creating an implicit key which is the hash of the traceback, this way the first and second checkbox will have different keys. I'll try sending a PR for this and we can see whether it makes sense (the concern with hashing a traceback is that it could add a non-trivial runtime overhead for each component instance).

wwwillchen commented 1 month ago

Actually the above issue is not specific to component tree diffs, it's something that's always been there - I'll open a separate issue for it (#312)