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

add deps and position field in VarData #4518

Merged
merged 21 commits into from
Dec 13, 2024

Conversation

Lendemor
Copy link
Collaborator

@Lendemor Lendemor commented Dec 10, 2024

When rendering hooks, some use case are not covered with the current hooks API.

This PR add two fields, deps and position to VarData that allow some controls on how to render hooks, both in relation to the useCallback generated by StatefulComponent.

Problem 1:

  • if you are writing a signature spec for an event, and this one depend on a hook you wrote in add_hooks, when wrapped in useCallback, your event handler will not update its data if the hook_name is not added to the dependency array.
    Solution: Passing the name of the hook in the new deps props.

Problem 2:

  • When adding a hook, you might either want:
    • to reference your hook inside an event handler (as a deps for example), so the hook need to be defined BEFORE.
    • to reference a memoized event handler inside your hook, so the hook need to be defined AFTER. (see clipboard.py)
      Solution: Inside a hook, pass either HooksPosition.PRE_TRIGGER or HooksPosition.POST_TRIGGER to the position props.

reflex/components/component.py Show resolved Hide resolved
reflex/vars/base.py Show resolved Hide resolved
@Lendemor Lendemor changed the title fix memoized event trigger order add deps and position field in VarData Dec 12, 2024
masenf
masenf previously approved these changes Dec 12, 2024
Copy link
Collaborator

@masenf masenf left a comment

Choose a reason for hiding this comment

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

i'm overall not crazy about packing more stuff in VarData because these fields do seem to have a pretty niche use in the framework... but i don't have an alternate suggestion that's better, so i think we can be pragmatic and take this.

reflex/components/component.py Outdated Show resolved Hide resolved
@Lendemor
Copy link
Collaborator Author

i'm overall not crazy about packing more stuff in VarData because these fields do seem to have a pretty niche use in the framework... but i don't have an alternate suggestion that's better, so i think we can be pragmatic and take this.

I sort of agree on this, that's why at first I tried to pass the position to a HookVar with special logic, but in the end passing them through VarData made code much cleaner and easier to read.

Also even if they are niche, they won't impact the majority of people who don't need them so it should be fine.

@masenf masenf merged commit 1444421 into main Dec 13, 2024
41 checks passed
@masenf masenf deleted the lendemor/fix_order_of_hooks_and_memoized_evt_trigger branch December 13, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants