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

RFC: Rework error handling #267

Open
1 of 6 tasks
akphi opened this issue Jun 17, 2021 · 0 comments
Open
1 of 6 tasks

RFC: Rework error handling #267

akphi opened this issue Jun 17, 2021 · 0 comments
Labels
Component: Graph Manager Issues related to graph processing and management (including interaction with engine server) logic Studio Core Team Opened by a member of the Studio core team Type: Discussion Type: Refactor
Milestone

Comments

@akphi
Copy link
Contributor

akphi commented Jun 17, 2021

💬 Request for Comments

The way to handle exception/errors right now is not systematic and potentially can cause bugs to be uncaught. This is bad because we can throw errors at users and cause the app to crash unintentionally.

Context and Motivation

  • Use alertIllegalUnhandledError to assert uncaught exceptions are not crashing the app at component-level
  • Wrap try-catch around each call instead of a using a catch-all block for important logics: Refactor critical path to use an approach similar to that of finos/legend-sdlc for error-handling (make the code verbose but very clear to read - we have try-catch around each call that might pose a problem, we learn how SDLC server methods like buildException that will build the exception (for 404, 403, etc.) and bypass already built exception, i.e. MetadataException (in our case StudioError or ApplicationError) - This might be a substantial piece of work
  • Consider using error-boundary where appropriate to handle UI issues (probably we can do catch-all somewhere) - https://www.npmjs.com/package/react-error-boundary
  • Reconsider how we throw errors while processing/building the graph. Right now we throw whatever types of error and wrap them with GraphError outside, is this the right thing to do?
  • Give better logging event, right now we use very generic logging event like SDLC_PROBLEM, SETUP_PROBLEM, those events are clearly not useful at all, we should make them more specific and if we follow the refactor for try/catch usage as detailed above, we should be able to use a proper logging event for each try/catch block
  • Especially, to handle something like Bug: Authentication token expired and not re-fetched after opening the editor for a long time #334, we should have a method like handleNetworkError() that will handle 401, 404, 403 and the likes systematically.
@akphi akphi added the Studio Core Team Opened by a member of the Studio core team label Oct 29, 2021
@akphi akphi added this to the Marathon milestone Nov 18, 2021
@akphi akphi added the Component: Graph Manager Issues related to graph processing and management (including interaction with engine server) logic label Feb 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Graph Manager Issues related to graph processing and management (including interaction with engine server) logic Studio Core Team Opened by a member of the Studio core team Type: Discussion Type: Refactor
Projects
None yet
Development

No branches or pull requests

1 participant