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.

Sign up for GitHub

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

Jump to bottom

fix: properly handle render errors in 'react' driver. #335

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: properly handle render errors in 'react' driver. #335

wants to merge 4 commits into from

Conversation

tmilar
Copy link

@tmilar tmilar commented Feb 2, 2021

With this change, a react component that throws on the render() async method, will check first if the component is still mounted, and in that case attempt to rethrow the error inside the React render loop.

Fixes #334

@tmilar tmilar requested a review from a team as a code owner February 2, 2021 22:32
@tmilar tmilar mentioned this pull request Feb 2, 2021
Open
@bluepnume
Copy link
Collaborator

This looks great to me, thanks for taking this up! Can you add a couple of tests to verify the new behavior?

@tmilar
Copy link
Author

tmilar commented Feb 3, 2021

This looks great to me, thanks for taking this up! Can you add a couple of tests to verify the new behavior?

Thanks! I'll try to work on that when I get some free time.
Since I'm on Windows I've been having a really though time making the project work with all the npm scripts intended for UNIX environments :(

@tmilar tmilar force-pushed the fix/react-driver branch 2 times, most recently from f038f7e to 645faac Compare February 4, 2021 17:40
@bluepnume
Copy link
Collaborator

Totally, sorry about the poor Windows support. If there are any changes you would like to include to improve cross-compatibility there, I'd be happy to merge them in.

@tmilar tmilar force-pushed the fix/react-driver branch 2 times, most recently from 06ef90e to cf61b10 Compare February 5, 2021 00:01
@danwetherald
Copy link

When will this be released? Seems like a major issue that is not being taken seriously with the priority it should have.

@gregjopa gregjopa mentioned this pull request Mar 11, 2021
Merged
@bluepnume bluepnume force-pushed the master branch 5 times, most recently from a6dbeec to f8eeb3e Compare September 10, 2021 07:52
@bluepnume bluepnume force-pushed the master branch 2 times, most recently from 22cbfdf to c890107 Compare September 22, 2021 06:36
@badeAdebayo
Copy link

will this ever be merged? @tmilar @bluepnume

@westeezy westeezy mentioned this pull request Feb 24, 2022
Open
@mdentremont mdentremont mentioned this pull request Apr 4, 2023
Open
@yongzhihuang
Copy link

Ping @tmilar @bluepnume ^^ 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing the container element after CLOSE/DESTROY zoid event (after close()), results in uncaught error
5 participants
@tmilar @bluepnume @danwetherald @badeAdebayo @yongzhihuang