posit-dev / py-shiny

Shiny for Python
https://shiny.posit.co/py/
MIT License
1.22k stars 69 forks source link

Trouble with reactive.Value containing a mutable object #588

Open nealrichardson opened 1 year ago

nealrichardson commented 1 year ago

I've run into some odd behavior and I'm hoping it's user error. I've created a simple app with a reactive.Value() containing a list. Some events trigger reactives that mutate that list (because lists are mutable), and I can see that the list is getting modified, but a UI element that should show the current state of the list isn't being updated. Code below, and working example on shinylive.

from shiny import reactive, render, ui, App

app_ui = ui.page_fluid(
    ui.row(
        ui.column(
            2,
            ui.row(
                ui.input_action_button("add_value", "Add to list"),
            ),
            ui.row(
                ui.input_action_button("remove_value", "Remove from list"),
            ),
        ),
        ui.column(
            10,
            ui.tags.div(id="module_container"),
        ),
    ),
    ui.output_text_verbatim("all_values")
)

def server(input, output, session):
    value_list = reactive.Value([])

    @reactive.Effect
    @reactive.event(input.add_value)
    def _():
        id = "value_" + str(len(value_list.get()))
        ui.insert_ui(
            selector="#module_container", where="afterBegin", ui=ui.input_numeric(id, id, 0)
        )
        value_list.get().append(getattr(input, id))

    @reactive.Effect
    @reactive.event(input.remove_value)
    def _():
        ui.remove_ui(selector=f"#module_container .shiny-input-container:first-child")
        value_list.get().pop()

    @output
    @render.text
    def all_values():
        return str(value_list())

app = App(app_ui, server)

all_values() depends on value_list(), and I'm changing the contents of value_list but all_values() isn't updating, even though I can see that the list inside is growing when I click "add to list" because the element label is a function of the length of the list.

gshotwell commented 1 year ago

There are two issues here as I see it.

First, you're modifying a reactive value in place with append and pop. This will cause unexpected behaviour and instead you should create a new list and set it with values.list.set(new_val)

Second, there's something funny about storing values produced by inserting and removing UI elements. If I understand correctly, what you're trying to do here is:

This is a bit of an anti-pattern because insert and remove ui don't do a good job of maintaining the reactive wiring between components. Instead I would recommend using dynamic UIs. To do this:

This is a bit less efficient for some things because the UI elements are rendered from scratch each time, but is easier to maintain and ensure application correctness.

nealrichardson commented 1 year ago

Thanks @GShotwell, that's spot on. https://shiny.rstudio.com/py/docs/reactive-mutable.html even shows my exact problem. I looked at the docs for reactive.Value but didn't see any warning of this--maybe we can add a xref in https://github.com/rstudio/py-shiny/blob/main/shiny/reactive/_reactives.py#L40-L81 that points to reactive-mutable.html?

nealrichardson commented 1 year ago

This is a bit less efficient for some things because the UI elements are rendered from scratch each time, but is easier to maintain and ensure application correctness.

Yeah that's how I ended up here, trying to separate out all of the independently reactive bits so that the whole thing didn't have to re-render every time--IIUC that's what Shiny should give me (particularly in contrast to something like Streamlit).