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 the possibility to pass custom front- and backend event exception handlers to the rx.App #3476

Closed

Conversation

maximvlah
Copy link
Contributor

@maximvlah maximvlah commented Jun 11, 2024

All Submissions:

  • Have you followed the guidelines stated in CONTRIBUTING.md file?
  • Have you checked to ensure there aren't any other open Pull Requests for the desired changed?

Type of change

  • New feature (non-breaking change which adds functionality)
  • This change requires a documentation update

New Feature Submission:

  • Does your submission pass the tests?
  • Have you linted your code locally prior to submission?

Changes To Core Features:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your core changes, as applicable?
  • Have you successfully ran tests with your changes locally?
#Test App

import reflex as rx
import traceback

class TestAppState(rx.State):
    """State for the TestApp app."""

    def divide_by_number(self, number: int):
        """Divide by number and print the result.

        Args:
            number: number to divide by
        """
        print(1 / number)


def frontend_exception_handler(exception: Exception):
    """Frontend exception handler."""

    print(f"Frontend exception: {exception}")

def backend_exception_handler(exception: Exception):
    """Backend exception handler."""

    error = traceback.format_exc()

    print(f"Backend exception: {error}")


app = rx.App(
    state=rx.State, 
    frontend_exception_handler=frontend_exception_handler, 
    backend_exception_handler=backend_exception_handler
)

@app.add_page
def index():
    return rx.vstack(
        rx.button(
            "induce_frontend_error",
            on_click=rx.call_script("induce_frontend_error()"),
            id="induce-frontend-error-btn",
        ),
        rx.button(
            "induce_backend_error",
            on_click=lambda: TestAppState.divide_by_number(0),  # type: ignore
            id="induce-backend-error-btn",
        ),
    )
    

closes #3284

@maximvlah maximvlah marked this pull request as ready for review June 11, 2024 14:43
@maximvlah maximvlah marked this pull request as draft June 11, 2024 19:20
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.

thanks for the contribution. i'll test it out further this week

reflex/state.py Show resolved Hide resolved
@maximvlah maximvlah marked this pull request as ready for review June 14, 2024 06:43
Copy link
Collaborator

@Lendemor Lendemor left a comment

Choose a reason for hiding this comment

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

I've started testing it on my side.

No issue with the backend error handler, seems to work properly.

I've had issue triggering it for frontend error however, seems like not all errors are being caught?

The following code still produce a frontend error, but doesn't seems to send it to the backend.
ie [Reflex Frontend Exception] does not appear in that case.

class MockCompo(rx.Component):
    tag = "UnknownTag"

@rx.page()
def index():
    return rx.foreach([1, 2, 3], MockCompo.create)

Also tested with

@rx.page()
def index():
    return rx.script(console.log(foo))

In this case it also doesn't triggers.

Also tested with

@rx.page()
def index():
    return rx.button("Test", on_click=rx.call_script("console.log(foo)"))

In this case it does trigger the frontend handler.

@Lendemor
Copy link
Collaborator

Lendemor commented Jun 18, 2024

Also, more importantly, I think the default handler should be tied to the App, not to the Config.

Config should stay as simple as possible.

We could take the PR as-is and extend it after to cover all the errors it doesn't catch at the moment, but the defaults function should be defined in the App.

@maximvlah
Copy link
Contributor Author

maximvlah commented Jun 18, 2024

@Lendemor thanks for the feedback. I will continue working on handling aforementioned edge cases tomorrow and will keep you updated. In the meantime, I have just moved the exception handlers from rx.Config to rx.App as you demanded.

Lendemor
Lendemor previously approved these changes Jun 18, 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.

This seems to work fine in dev mode (although a frontend error triggers the handler twice for some reason).

But in prod mode, when i hit an unhandled frontend error, it does not fire the event...

import reflex as rx


class State(rx.State):
    broken: bool = False


def index() -> rx.Component:
    return rx.container(
        rx.vstack(
            rx.cond(
                State.broken,
                rx.text("Broken", size=rx.Var.create("x", _var_is_local=False, _var_is_string=False)),
            ),
            rx.text("Break the app? ", rx.switch(checked=State.broken, on_change=State.set_broken)),
            spacing="5",
            justify="center",
            min_height="85vh",
        ),
        rx.logo(),
    )


app = rx.App()
app.add_page(index)

Flipping the switch introduces a component with a rendering error.

In dev mode, flipping the switch prints the error to the terminal, browser console, and on screen.

In prod mode, flipping the switch redirects to a page that says "Application error: a client-side exception has occurred (see the browser console for more information)."

reflex/state.py Outdated Show resolved Hide resolved
@maximvlah
Copy link
Contributor Author

maximvlah commented Jun 20, 2024

I was able to fix the issue mentioned by @masenf with error boundary wrapper.

I used the react-error-boundary lib recommended by React team:
https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary

This made it catch these errors in production and render a fallback component, however in debug mode it fired the error handler thrice. So, I made the error boundary call onError only in production through checking the process.env.NODE_ENV. Still we have the bug with frontend errors fired twice in debug mode which according to facebook/react#11499 is a known issue with no trivial solution.

Also the ErrorBoundary has limitations, i.e. does not catch all unhandled errors. https://legacy.reactjs.org/docs/error-boundaries.html
Therefore, in my opinion, using the window.onerror logic which I introduced initially together with ErrorBoundary may enable us to catch most of the frontend errors. This is all hacky and I am not fully satisfied with this solution, however I haven’t found a better one yet. If we go for it, it would be great to allow users to define the fallback component as well as onError and onReset behaviour from python side. I followed the docs and tried wrapping ErrorBoundary in Reflex, however had little success, especially with the callbacks. I would be very grateful if you could help me properly wrap it. This is what I tried:

class ErrorBoundary(Component):
    """A React Error Boundary component that catches unhandled frontend exceptions."""

    library = "react-error-boundary"
    tag = "ErrorBoundary"

    def _render(self, props: dict[str, Any] | None = None) -> Tag:
        """Define how to render the component in React.

        Args:
            props: The props to render (if None, then use get_props).

        Returns:
            The tag to render.

        """
        # Create the base tag.
        tag = Tag(
            name=self.tag if not self.alias else self.alias,
            special_props=self.special_props,
        )

        if props is None:
            # Add component props to the tag.
            props = {
                attr[:-1] if attr.endswith("_") else attr: getattr(self, attr)
                for attr in self.get_props()
            }

            # Add ref to element if `id` is not None.
            ref = self.get_ref()
            if ref is not None:
                props["ref"] = Var.create(
                    ref, _var_is_local=False, _var_is_string=False
                )
        else:
            props = props.copy()

        props.update(
            **{
                trigger: handler
                for trigger, handler in self.event_triggers.items()
                if trigger not in {EventTriggers.ON_MOUNT, EventTriggers.ON_UNMOUNT}
            },
            key=self.key,
            id=self.id,
            class_name=self.class_name,
        )
        props.update(self._get_style())
        props.update(self.custom_attrs)

        # remove excluded props from prop dict before adding to tag.
        for prop_to_exclude in self._exclude_props():
            props.pop(prop_to_exclude, None)

        props["onError"] = Var.create_safe(
            "logFrontendError", _var_is_string=False, _var_is_local=False
        )
        props["FallbackComponent"] = Var.create_safe(
            "Fallback", _var_is_string=False, _var_is_local=False
        )

        return tag.add_props(**props)

    def _get_events_hooks(self) -> dict[str, None]:
        """Get the hooks required by events referenced in this component.

        Returns:
            The hooks for the events.
        """
        return {Hooks.FRONTEND_ERRORS: None}

Regarding the errors mentioned by @Lendemor, I think they both occur before the event loop (on which all my logic depends) is instantiated and thus the frontend error events are not propagated to the python side.

p.s. rx.script("console.log(foo)", strategy="lazyOnload") is caught

Please, check out the latest commit I made just now and let me know what you think.

Copy link
Contributor

@benedikt-bartscher benedikt-bartscher left a comment

Choose a reason for hiding this comment

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

@maximvlah what about exceptions in reflex.app._process?
See Line 1162

Do we want to combine exception handlers with telemetry.send_error? Maybe replace all occurrences of telemetry.send_error with the backend exception handler and let telemetry.send_error be part of the default exception handler?

@maximvlah
Copy link
Contributor Author

maximvlah commented Jun 25, 2024

@benedikt-bartscher I must have missed it, just addded backend exception handler to _process, thanks for pointing it out.
I think it is better to keep telemetry separate as if the user overrides the backend exception handler and does not include telemetry.send_error in its body and the telemetry is enabled, then it would not be sent to reflex team.

@benedikt-bartscher
Copy link
Contributor

6cd628f introduced a lot of formatting, iirc this has already been done on main. Please rebase/merge main or cleanup this commit

@Lendemor
Copy link
Collaborator

Lendemor commented Jun 25, 2024

6cd628f introduced a lot of formatting, iirc this has already been done on main. Please rebase/merge main or cleanup this commit

These changes are not on main, but they shouldn't be because we've added that rule to lint.ignore in pyproject.toml.
Not sure how ruff was run on that PR, but it should pick up pyproject.toml normally.

@maximvlah
Copy link
Contributor Author

Please rebase/merge main or cleanup this commit

@benedikt-bartscher Done

@maximvlah maximvlah changed the title Add the possibility to pass custom front- and backend event exception handlers to the rx.Config Add the possibility to pass custom front- and backend event exception handlers to the rx.App Jun 25, 2024
@Lendemor
Copy link
Collaborator

Lendemor commented Jun 25, 2024

There is still too many modified files, the additional blank lines in the docstrings shouldn't be there.

@maximvlah maximvlah closed this Jun 26, 2024
@maximvlah maximvlah deleted the resolve-conflicts-with-upstream branch June 26, 2024 08:11
@maximvlah maximvlah restored the resolve-conflicts-with-upstream branch June 26, 2024 08:12
@maximvlah maximvlah reopened this Jun 26, 2024
@maximvlah
Copy link
Contributor Author

maximvlah commented Jun 26, 2024

@Lendemor @benedikt-bartscher ok, it looks like it is clean now, sorry for the mess. I want to rename the branch to a more meaningful name like add-exception-handlers. I have just tried it from github web like here however it closed the PR. Is there another way or should I just leave like this? Thanks!

@masenf
Copy link
Collaborator

masenf commented Jun 26, 2024

@maximvlah i'm still seeing the blank line changes in many files. you might try just opening a new PR with your renamed and fixed branch and i'll test it out and review it. we really want to get this feature in and appreciate you working on it 😄

@maximvlah
Copy link
Contributor Author

@masenf ok, will do it now

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.

[REF-2825] Need a mechanism to catch unhandled errors from event handlers
4 participants