Skip to content
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

[ENG-4153]Use empty string for None values in rx.input and rx.el.input #4521

Merged
merged 10 commits into from
Dec 12, 2024
28 changes: 28 additions & 0 deletions reflex/components/el/elements/forms.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
prevent_default,
)
from reflex.utils.imports import ImportDict
from reflex.utils.types import is_optional
from reflex.vars import VarData
from reflex.vars.base import LiteralVar, Var

Expand Down Expand Up @@ -384,6 +385,33 @@ class Input(BaseHTML):
# Fired when a key is released
on_key_up: EventHandler[key_event]

@classmethod
def create(cls, *children, **props):
"""Create an Input component.

Args:
*children: The children of the component.
**props: The properties of the component.

Returns:
The component.
"""
from reflex.vars.number import ternary_operation

value = props.get("value")

# React expects an empty string(instead of null) for uncontrolled inputs.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: aren't these controlled inputs when a value is passed

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah yeah, good catch

if value is not None and is_optional(
(value_var := Var.create(value))._var_type
):
props["value"] = ternary_operation(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like adding a ternary to every value prop on every text input is a bit excessive. maybe we can check isoptional(value._var_type) and apply the ternary in that case.

This does potentially create an issue where the Var is not typed optional, but still ends up with a None value, however the fix for that is to improve the annotation of the Var.

I know it's probably negligible and maybe not worth optimizing for, but perhaps we should profile text field re-render time with and without a ternary in the value?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like adding a ternary to every value prop on every text input is a bit excessive. maybe we can check isoptional(value._var_type) and apply the ternary in that case.

This would be great and keep the generated code simpler and faster for values which will never be None

This does potentially create an issue where the Var is not typed optional, but still ends up with a None value, however the fix for that is to improve the annotation of the Var.

I agree, we should not bother too much about wrongly annotated vars.

I know it's probably negligible and maybe not worth optimizing for, but perhaps we should profile text field re-render time with and without a ternary in the value?

While profiling is a great idea, I think it's a bit overkill if we only do this for optional values. However, I do not want to stop anyone from doing that :D

(value_var != Var.create(None)) # pyright: ignore [reportGeneralTypeIssues]
& (value_var != Var(_js_expr="undefined")),
value,
Var.create(""),
)
return super().create(*children, **props)


class Label(BaseHTML):
"""Display the label element."""
Expand Down
4 changes: 2 additions & 2 deletions reflex/components/el/elements/forms.pyi
Original file line number Diff line number Diff line change
Expand Up @@ -512,7 +512,7 @@ class Input(BaseHTML):
on_unmount: Optional[EventType[[], BASE_STATE]] = None,
**props,
) -> "Input":
"""Create the component.
"""Create an Input component.

Args:
*children: The children of the component.
Expand Down Expand Up @@ -576,7 +576,7 @@ class Input(BaseHTML):
class_name: The class name for the component.
autofocus: Whether the component should take the focus once the page is loaded
custom_attrs: custom attribute
**props: The props of the component.
**props: The properties of the component.

Returns:
The component.
Expand Down
15 changes: 15 additions & 0 deletions reflex/components/radix/themes/components/text_field.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,9 @@
from reflex.components.core.debounce import DebounceInput
from reflex.components.el import elements
from reflex.event import EventHandler, input_event, key_event
from reflex.utils.types import is_optional
from reflex.vars.base import Var
from reflex.vars.number import ternary_operation

from ..base import LiteralAccentColor, LiteralRadius, RadixThemesComponent

Expand Down Expand Up @@ -96,6 +98,19 @@ def create(cls, *children, **props) -> Component:
Returns:
The component.
"""
value = props.get("value")

# React expects an empty string(instead of null) for uncontrolled inputs.
if value is not None and is_optional(
(value_var := Var.create(value))._var_type
):
props["value"] = ternary_operation(
(value_var != Var.create(None)) # pyright: ignore [reportGeneralTypeIssues]
& (value_var != Var(_js_expr="undefined")),
value,
Var.create(""),
)

component = super().create(*children, **props)
if props.get("value") is not None and props.get("on_change") is not None:
# create a debounced input if the user requests full control to avoid typing jank
Expand Down
Loading