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

Improve Tab Error handling #1214

Closed
3 tasks done
nikku opened this issue Feb 12, 2019 · 4 comments
Closed
3 tasks done

Improve Tab Error handling #1214

nikku opened this issue Feb 12, 2019 · 4 comments
Assignees
Milestone

Comments

@nikku
Copy link
Member

nikku commented Feb 12, 2019

Is your feature request related to a problem? Please describe.

Tabs may throw errors and technically we got the EditorTab to handle errors gracefully and propagate them. Unfortunately triggering an actual error, e.g. by inserting a line such as

throw new Error('FOO');

into a tab constructor, e.g. the BpmnTab is not properly handled and results in the app being unmounted.

Describe the solution you'd like

  • Tab errors are handled gracefully
  • Logged to log panel
  • A error placeholder is shown to the user in place of the defective tab

Describe alternatives you've considered

None.

@nikku nikku added this to the M28 milestone Feb 12, 2019
@pinussilvestrus pinussilvestrus added the ready Ready to be worked on label Feb 13, 2019
@barmac barmac self-assigned this Feb 28, 2019
@barmac barmac added in progress Currently worked on and removed ready Ready to be worked on labels Feb 28, 2019
@barmac
Copy link
Collaborator

barmac commented Feb 28, 2019

The reason why we don't handle errors gracefully is that EditorTab is an incomplete error boundary. Namely, it does not implement static getDerivedStateFromError.
Cf. https://reactjs.org/docs/error-boundaries.html; also this codesandbox https://codesandbox.io/s/202l1xyk0r?fontsize=14

@nikku
Copy link
Member Author

nikku commented Feb 28, 2019

The reason why we don't handle errors gracefully is that EditorTab is an incomplete error boundary. Namely, it does not implement static getDerivedStateFromError.

We don't have to implement getDerivedStateFromError. It should be sufficient to unmount the broken editor (replacing it with an ErrorTab or the like).

@nikku
Copy link
Member Author

nikku commented Feb 28, 2019

Suggestion: Find an icon to make someones day, if the editor fails, e.g. https://thenounproject.com/term/open-source/?search=error

Caveat: We need to make sure we use free icons or provide proper attribution.

@barmac
Copy link
Collaborator

barmac commented Mar 4, 2019

Note: Enzyme does not support getDerivedStateFromError yet (enzymejs/enzyme#1553 (comment)). Because of this, the tests will be a bit different from what we already have in our code base.

@nikku nikku closed this as completed in e3815ce Mar 5, 2019
@ghost ghost removed the in progress Currently worked on label Mar 5, 2019
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