Dynamic route vars silently shadow all other vars

[benedikt-bartscher]

No description provided.

[abulvenz]

Thanks for fixing. Here is my broken almost minimal example:

import reflex as rx

app = rx.App()


class State1(rx.State):
    redirect_to: str | None = None
    redirect_me: str | None = None

    def on_load(self):
        self.redirect_to = self.router.page.params.get("redirect_me")
        self.redirect_me = self.router.page.params.get("redirect_me")
        print(self)
        return rx.redirect(f"/{self.redirect_me}")


@rx.page(route="/goto/[redirect_me]", on_load=State1.on_load)
def goto() -> rx.Component:
    return rx.box(
        rx.text("Redirecting..."),
    )


@rx.page(route="/page2")
def page2() -> rx.Component:
    return rx.box(
        rx.heading("Page 2"),
        rx.text("This is page 2"),
        rx.el.pre(
        rx.text(f"Different name from route param  >>{State1.redirect_to}<<"),
        rx.text(f"Same name as route param is gone >>{State1.redirect_me}<<"),
        )
    )


def index() -> rx.Component:
    return rx.box(
        rx.heading("Hello, Reflex!"),
        rx.text("This is a test of Reflex."),
        rx.button("Go to /goto/page2", on_click=rx.redirect("/goto/page2")),
    )


app.add_page(index)

[benedikt-bartscher]

There was one minor issue with multiple pages using the same dynamic argument name. It is fixed and tested now

[benedikt-bartscher]

I would like to deprecate those automatically generated computed vars for dynamic routes.

• They are untyped because the attribute is added dynamically
• They can be easily created manually
• There should be one good way to do things
• They either shadow other vars or block var names (this PR)
• They are uncached computed vars, and without usage tracking they are computed and transmitted to the frontend all the time, even if you don't need them

What's your opinion @picklelo?

[masenf]

I would like to deprecate those automatically generated computed vars for dynamic routes.

I could get on board with this. In most of my apps that use dynamic route vars, i typically have to add them to my state manually anyway, otherwise they cannot be referenced in some situations (like setting the page title for app.add_page).

I wonder if it makes sense to dynamically add route vars to a specific substate, rather than the root state 🤔 Something like rx.Query.redirect_me. I know we have rx.State.router.page.params["redirect_me"], but that's kind of a lot to type. Although i guess if we're going to do that, then we should just have rx.Query implement __getattr__ to return the full thing, then it would always be defined, even before the compile runs.

The other big problem I see in state.py is that substates cannot redefine or override fields from a parent state. Maybe this restriction doesn't make sense anymore... if a substate redefines a field, then it should be a different value than the same-named field in a parent state, but that's just not how the code is written today.

[benedikt-bartscher]

Thanks, @masenf, for the great ideas.
I would merge this PR as-is to prevent unwanted shadowing and create issues for further dynamic arg var decisions and substate field overwrites.

Something like rx.Query.redirect_me

I am unsure about this because it stores and transmits the same variable multiple times. All query params still live in rx.State.router. Maybe we should focus on #3681 first and later add a RouterState.params hybrid_property (#3806) to add an ergonomic alias without overhead.

The other big problem I see in state.py is that substates cannot redefine or override fields from a parent state

I totally agree 💯 I think we should change this
Edit: I just opened #3820 for this

[masenf]

Instead of adding a field to ImmutableComputedVar, can we subclass it as DynamicRouteVar and use that as the determinant?

[benedikt-bartscher]

Instead of adding a field to ImmutableComputedVar, can we subclass it as DynamicRouteVar and use that as the determinant?

Great idea, done