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 error boundaries [in-progress by @kaushlakers] #1351

Closed
bvaughn opened this issue Aug 28, 2017 · 9 comments
Closed

Add error boundaries [in-progress by @kaushlakers] #1351

bvaughn opened this issue Aug 28, 2017 · 9 comments

Comments

@bvaughn
Copy link
Contributor

bvaughn commented Aug 28, 2017

Since the REPL is already using React 16, we should take advantage of the new componentDidCatch error handling lifecycle.

As we move forward on issues like #858 and #1292, it would be nice to add componentDidCatch to a couple of key components. The REPL currently uses try/catch statements where errors are likely to occur (eg compile, eval) but error boundaries would be a nice backup for any cases that slip through the cracks.

@bvaughn bvaughn added this to the New REPL milestone Aug 28, 2017
@kaushlakers
Copy link
Contributor

I can take a stab at this if that's sounds okay. :) I'm a complete newbie to OSS, but I was able to understand "most" of the words in the description, so it felt doable.

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 3, 2017

Have at it, @kaushlakers! Let me know when you're ready for me to take a look.

@kaushlakers
Copy link
Contributor

Awesome! I'll dig in and come back here with more clarifications 😃

@bvaughn bvaughn changed the title Add error boundaries Add error boundaries [in-progress by @kaushlakers] Sep 3, 2017
@kaushlakers
Copy link
Contributor

@bvaughn after digging in a bit, looks like there's 2 kinds of errors that are dealt with. One is when babel fails to load, in which case, we just display an error message. Other case is the errorMessage we display in the CodeMirrorPanel when compiles/evals fail. Do we want to display a backup error message for these unknown errors in the CodeMirrorPanel since the assumption is that babel has properly loaded already? Also please let me know if I'm completely on the wrong path here 😃

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 4, 2017

I was thinking we might just add a top-level error boundary (to the Repl component, for example) that basically showed a similar error message as we do when Babel fails to load. Just a catch-all so we don't just crash/unmount without explanation if an unexpected error occurs.

I've added error handling in the various places I think errors are likely to occur. This would just be to catch ones I hadn't anticipated. 😄

@kaushlakers
Copy link
Contributor

Sounds good! 👍

kaushlakers pushed a commit to kaushlakers/website that referenced this issue Sep 4, 2017
@kaushlakers
Copy link
Contributor

@bvaughn your move 😄 Thanks!

@bvaughn bvaughn closed this as completed in e90a830 Sep 6, 2017
@kaushlakers
Copy link
Contributor

Great! Happy to help on other tasks too, if needed. 😃

@bvaughn
Copy link
Contributor Author

bvaughn commented Sep 6, 2017

Cool! Check out some other beginner friendly or help wanted tasks if you're looking for ideas. 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants