reflex-dev / reflex

πŸ•ΈοΈ Web apps in pure Python 🐍
https://reflex.dev
Apache License 2.0
18.02k stars 997 forks source link

rx.debounce_input() does not work with rx.slider() #3038

Open straygar opened 3 months ago

straygar commented 3 months ago

Describe the bug

When using a controlled rx.slider(), I want the on_change handler to be de-bounced, to have both:

  1. a responsive UI
  2. protect the backend from spam

I have tried using on_value_commit, but it is pretty flakey. See Radix UI issue: https://github.com/radix-ui/primitives/issues/1760

Instead, I opted to try rx.debounced_input(), but it is not compatible with rx.slider().

I suspect debounce_input() is intended to work with semantic <input> HTML elements, but Radix UI opted for their own funky implementation (a bunch of span/p/divs). πŸ€·β€β™€οΈ

To Reproduce Steps to reproduce the behavior:

value = State.my_values[name]
min_val = 0
max_val = 100
rx.debounce_input(
    rx.slider(
        value=[value],
        on_change=lambda val: State.update_my_value(name, val, True),
        min=min_val,
        max=max_val,
        step=1,
    ),
    debounce_timeout=2_000,
)

If I specify a value on rx.slider(), I get:

TypeError: Invalid var passed for prop value, expected type <class 'str'>, got value [state__state.my_values[name]] of type <class 'list'>.

If I omit it, I get a react compilation error:

Unhandled Runtime Error
TypeError: (intermediate value).map is not a function
Call Stack
Slider<
node_modules/@radix-ui/themes/dist/esm/components/slider.js (14:0)
renderWithHooks
node_modules/react-dom/cjs/react-dom.development.js (16305:0)
updateForwardRef
node_modules/react-dom/cjs/react-dom.development.js (19226:0)
beginWork
node_modules/react-dom/cjs/react-dom.development.js (21636:0)
callCallback
node_modules/react-dom/cjs/react-dom.development.js (4164:0)
invokeGuardedCallbackDev
node_modules/react-dom/cjs/react-dom.development.js (4213:0)
invokeGuardedCallback
node_modules/react-dom/cjs/react-dom.development.js (4277:0)
beginWork$1
node_modules/react-dom/cjs/react-dom.development.js (27451:0)
performUnitOfWork
node_modules/react-dom/cjs/react-dom.development.js (26557:0)
workLoopSync
node_modules/react-dom/cjs/react-dom.development.js (26466:0)
renderRootSync
node_modules/react-dom/cjs/react-dom.development.js (26434:0)
performConcurrentWorkOnRoot
node_modules/react-dom/cjs/react-dom.development.js (25738:0)
workLoop
node_modules/scheduler/cjs/scheduler.development.js (266:0)
flushWork
node_modules/scheduler/cjs/scheduler.development.js (239:0)
performWorkUntilDeadline
node_modules/scheduler/cjs/scheduler.development.js (533:0)

Expected behavior Either:

Specifics (please complete the following information):

masenf commented 3 months ago

There are two problems going on here:

  1. the type of value in DebounceInput is Var[str], and it should be Var[Any] so that any underlying API can be used
  2. The other problem was below that:

Reflex actually renames the prop onChange to onValueChange for the radix Slider element, but the debounce doesn't take this into account, and so it never actually gets the sliders onValueChange event fired and thus never updates. This is actually much harder to fix...

On the plus side, we are considering adding a reflex native "event throttling" mechanism. Similar to debounce, but actually to rate limit the number of events sent to the backend in a given time interval. We don't have a firm date for when such a feature will be available though.

I don't see an easy fix or workaround for this at the moment.

straygar commented 3 months ago

Thanks for the quick answer!

I haven't tried implementing a custom UI component in Reflex yet, but could a valid workaround (assuming the 1st point you mention gets fixed) be to implement my own <input type="range"> based component, and wrap it in a debounce_input()?

EDIT: although that might also be problematic without using raw rx.html(), as rx.input() seems to assume a textual input (as it adds classes like rt-TextFieldInput etc.).

Lendemor commented 3 months ago

EDIT: although that might also be problematic without using raw rx.html(), as rx.input() seems to assume a textual input (as it adds classes like rt-TextFieldInput etc.).

We also have the el namespace for basic HTML element, so you can maybe extend from rx.el.input instead