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

fix: validate event #338

Merged
merged 2 commits into from
Jan 27, 2023
Merged

fix: validate event #338

merged 2 commits into from
Jan 27, 2023

Conversation

qu3vipon
Copy link
Contributor

@qu3vipon qu3vipon commented Jan 25, 2023

Resolve: #310

Validate EventChain explicitly, so other types will be caught as a ValueError

ValueError: Invalid event chain: {state. some_var}

@@ -158,7 +158,7 @@ def _create_event_chain(
ValueError: If the value is not a valid event chain.
"""
# If it's already an event chain, return it.
if isinstance(value, Var):
if isinstance(value, EventChain):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think here the code is correct but the comment is incorrect.

Can we revert the code, but fix the comment to say:

# If it's a var, return it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe in addition, we need the EventChain check? But we definitely need this current one. What code did you run that raised the ValueError?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

EventChain check is for CustomComponent.

I run the same code from #310.

class State(pc.State):
    some_var: bool

def index():
    return pc.box(pc.icon(tag="AddIcon", on_click=State.some_var))

some_var is passed as a BaseVar, it is a child class of Var, so it is caught at first if statement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain more about why we definitely need the current one when we create EventChain? BaseVar shouldn't be returned in my opinion.

Copy link
Contributor

@picklelo picklelo Jan 27, 2023

Choose a reason for hiding this comment

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

Ahh I see - you're right this is causing the other bug it looks like. We need this line for the CustomComponent feature that I'm working on (it's not released yet, but it will allow you to create memoized React components for better performance).

But I think in adding that line it caused this other bug. Let me think a bit more on how to resolve this.

Basically for CustomComponent it can take in a Var, but for regular components it should not allow it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe we can add a line like

if isinstance(self, CustomComponent):
    if isinstance(value, Var):
        return value

Then it should fix this bug, but still work for the new feature.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Should I change the code as you suggest? or just close this pr and wait for your new feature?

Copy link
Contributor Author

@qu3vipon qu3vipon Jan 27, 2023

Choose a reason for hiding this comment

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

I just did a quick fix as you suggest. Thanks for the help.

Copy link
Contributor

@picklelo picklelo left a comment

Choose a reason for hiding this comment

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

thanks for the fix!

@picklelo picklelo merged commit 1c20f43 into reflex-dev:main Jan 27, 2023
@qu3vipon qu3vipon deleted the fix/validate-event branch January 28, 2023 04:59
ACucos1 pushed a commit to ACucos1/rd-pynecone that referenced this pull request Feb 28, 2023
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.

Uncaught JS Error : Passing a var instead of event handler to an event_trigger
2 participants