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

Logging when set_state is called and no changes occur #565

Closed
Archmonger opened this issue Jan 4, 2022 · 7 comments
Closed

Logging when set_state is called and no changes occur #565

Archmonger opened this issue Jan 4, 2022 · 7 comments
Labels
priority-2-moderate Should be resolved on a reasonable timeline. type-feature About new capabilities
Milestone

Comments

@Archmonger
Copy link
Contributor

Archmonger commented Jan 4, 2022

Current Situation

Currently, there exists circumstances where set_state can be called and nothing will occur. For example, modifying a mutable class (such as a dataclass)

@dataclass
class MyState:
   value: str = ""

@idom.component
def my_component():
   state, set_state = idom.hooks.use_state(MyState())

   async def on_click(event):
      state.value = "dog"
      set_state(state)
      # This does not trigger a re-render. Need to use copy.copy(state).

   return button( {"onClick"=on_click}, ... )

Proposed Changes

Notify the user if set_state was called and no re-render was queued.

Implementation Details

When in IDOM_DEBUG_MODE...
Show either logging.warning or logging.info when set_state is called but a re-render was not trigged.

@Archmonger Archmonger added the flag-triage Not prioritized. label Jan 4, 2022
@rmorshea
Copy link
Collaborator

rmorshea commented Jan 4, 2022

React also has a useDebugValue hook. It would be fairly simple to implement something that logs when the value changes:

@component
def SomeComponent():
    is_online, set_is_online = use_state(False)

    use_debug_value("is online" if is_online else "is offline")

    ...

Which might log:

DEBUG: SomeComponent(10c0049a0) is online
DEBUG: SomeComponent(10c0049a0) is offline

@Archmonger
Copy link
Contributor Author

On a related note, does react have the same issues of mutability?

@rmorshea
Copy link
Collaborator

rmorshea commented Jan 5, 2022

Yes it does. There's a section in React's new documentation about it which I'm also writing for IDOM presently.

@rmorshea
Copy link
Collaborator

rmorshea commented Jan 5, 2022

React's doc suggest using immer to avoid some of these problems. The best solution to this problem in Python that I'm aware of is pyresistent (which I plan to call out in the docs).

@Archmonger
Copy link
Contributor Author

While I could see this being a challenge in JavaScript, this could be resolved within the IDOM framework.

From my perspective, I'm effectively calling set_state(copy.copy(my_object)) every time, unless the value is immutable. Immutable collections in pyresisant are effectively doing the same on every value mutation. This adds on unnecessary overhead.

Perhaps if issubclass(my_object, (collections.MutableSequence, collections.MutableSet, collections.MutableMapping)), then we can automatically force a re-render on every set_state. Or to mimic what is currently occurring, the state hook can call copy.copy(my_object), if old_state==new_state).

I don't think there's a need to inherit the same limitations as react within the Python space.

Thoughts?

@rmorshea
Copy link
Collaborator

rmorshea commented Jan 5, 2022

While it might feel like a limitation, this choice by React makes you think about mutations in the hope that it will encourage you to avoid them. While you can get away with mutating state and then forcing a render by setting a copy, it can introduce unexpected bugs. Consider that this:

new_list = old_list + [value]
# or this even
new_list = old_list.copy()
new_list.append(value)

Is subtlety different, from this:

old_list.append(value)
new_list = old_list.copy()

Because we've modified the old_list, anything which is holding onto that old_list might now be affected.

Here's a somewhat more concrete example... Imagine that you have some message form with a dropdown for selecting a recipient and a button for sending messages. The catch is that sending the message takes some time, and we need to await some things before it actually gets sent. Consider the code below and try to spot the bug:

@component
def MessageForm():
    state, set_state = use_state({
        "recipient": "Alice",
        "message": ""
    })

    async def handle_send(event):
        await some_long_task()
        await send_message(state["recipient"], state["message"])

    def handle_recipient_change(event):
        state["recipient"] = event["target"]["value"]
        new_state = state.copy()
        set_state(new_state)

    def handle_message_change(event):
        state["message"] = event["target"]["value"]
        new_state = state.copy()
        set_state(new_state)
    
    ....

There's a very subtle bug here where, if the user attempts to send the message (which takes some time) and then immediately changes the recipient, the message will get sent to the recipient they just changed to rather than the recipient which was selected at the time they tried to send. A similar bug would happen with the state["message"] as well. This is because we're modifying the same state instance which we use in the event handler to send_message(state["recipient"], state["message"]).

@rmorshea rmorshea added type-feature About new capabilities and removed flag-triage Not prioritized. labels Jan 9, 2022
@rmorshea rmorshea added this to the 2.0 milestone Jan 11, 2022
@rmorshea rmorshea added the priority-2-moderate Should be resolved on a reasonable timeline. label Jan 13, 2022
@rmorshea
Copy link
Collaborator

closing. this is addressed by #568 and the use_debug_value hook.

@Archmonger Archmonger modified the milestones: Luxury, 1.0 Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority-2-moderate Should be resolved on a reasonable timeline. type-feature About new capabilities
Projects
None yet
Development

No branches or pull requests

2 participants