-
Notifications
You must be signed in to change notification settings - Fork 1
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
exception handler draft #1
base: main
Are you sure you want to change the base?
Conversation
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Riyad, you are on the right track!
Btw, this part only takes care of exceptions thrown by event handlers. Client errors like in rx.call_script
are still not handled. However, that's not as trivial to implement. If you want to collect some bonus points, also catch those errors and send them back to the backend. If you do not fully understand this, no worries, I am always here to answer all your questions or create an example for you.
reflex/state.py
Outdated
|
||
@classmethod | ||
def _handle_exception(cls, exception): | ||
if cls._exception_handler: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should add a default exception handler which replicates the old behavior (just print()
errors) and drop this check
Btw @tanvirriyad please also take a look on those failing tests (pre-commit and unit-tests). The benchmarking job is expected to fail |
… exception is added
I looked at pre-commit failure. It happened for some docstring issues. I updated those. One unit test is failing because of return type issues. |
I have a small question regarding rx.call_script(). this function executes front-end code, right? when a user writes frontend code, they would be able to pass a custom exception handler as an argument like rx.App()? |
I wanted to review your changes later this evening, but since you seem to be interested in the This is an example reflex app which throws such an error in the frontend. You can just run it, open your chrome dev tools and checkout the JS console while pressing the test button. import reflex as rx
class State(rx.State):
def test_handler(self):
print("test_handler called")
yield rx.call_script("error")
def index() -> rx.Component:
return rx.button(
"Test",
on_click=State.test_handler, # type: ignore
)
app = rx.App()
app.add_page(index) As already mentioned the implementation is not trivial, you would have to send this error to the backend and pass it to the exception handler there. |
…ch frontend exception
@@ -668,19 +667,23 @@ def call_script( | |||
callback_kwargs = { | |||
"callback": f"({arg_name}) => queueEvents([{format.format_event(event_spec)}], {constants.CompileVars.SOCKET})" | |||
} | |||
wrapped_code = f""" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
No description provided.