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

Consistent Companion error API #5436

Open
mifi opened this issue Aug 29, 2024 · 2 comments
Open

Consistent Companion error API #5436

mifi opened this issue Aug 29, 2024 · 2 comments
Labels

Comments

@mifi
Copy link
Contributor

mifi commented Aug 29, 2024

Problem

Currently there is no proper, documented system in in place for passing errors from Companion to Uppy

Solution

  • Should pass only error codes and strictly specified error metadata to the client. We should not pass message strings from Companion to Uppy because messages need to be i18n in Uppy before being shown to the user.
  • Maybe use TypeScript to define our errors, error codes and metadata for easier handling. e.g. the Uppy dev can just catch an error if (err instanceof ValidationError) { markFormFieldsInRedColor(err.fields); showErrorToUser(i18nError(err.code)) }
  • Need to document the error API

Note that we have to be careful and try not to make this breaking changes between Uppy and Companion.

We also need to distinguish between expected and unexpected errors:

Expected errors

Expected errors are errors that are part of a normal flow and should not raise any alarm.
Examples:

  • Intermittent errors: Connection reset by peer, Failed to resolve dns, Lost internet connection
  • Validation errors like if a file is too large to upload to this destination
  • Upload canceled by user
  • Uploader/feature is not enabled for this server

These kinds of errors should either be:

  • returned using a generic error code for intermittent errors (e.g. ask the user to try again),
  • or returned using a specific error code that can be i18n for the user to see what is wrong and any actions they might need to do.

Related:

Unexpected errors

All other errors are errors that don't match "Expected errors". They should be logged as logger.error so that error reporting systems can pick them up and be actionable for Operations. These errors should be returned to the user with a generic error code (e.g. there is a temporary problem, please try again later).

Examples of such errors:

  • unable to connect to redis,
  • no route to host
  • out of memory, Maximum call stack size exceeded
  • failed to import ESM module
  • node:assert error
  • CORS error
  • most kinds of TypeError

Alternatives

Just use a simple API where all errors are treated equally and sent to the user as a "generic error" response (but logged on the server). Then all we can do in Uppy is to ask the user to try again (which might sometimes work, but sometimes not due to the nature of the error). Then the user would contact the company using Uppy which will make an issue here and we have to solve the issue.

@yaegor
Copy link
Contributor

yaegor commented Aug 29, 2024

Thank you for taking a generic approach to the error reporting.
I'd just add a use case from my original issue #4079: I do want to show a custom error message specific to my app to the user. I guess considering the i18n it should take reporting an error code (and idealy also some message or a set of attributes/values) in the server API and then on the client translating that error code and additional data to a custom message for the UI to show that to the user.
(updated - added "on the client")

@mifi
Copy link
Contributor Author

mifi commented Aug 29, 2024

Not sure if the backend should do the translation, because we already do that in the frontend. Maybe we can instead define a certain error type that the server that companion connects to can respond with, and companion will then forward them to uppy in the standard error structure. Maybe this error can contain a special error code (in a user-defined error code range or in a special vendorCode property). Then in Uppy we can provide a way to extend i18n by either:

  • allowing the developer to provide additional custom error i18n keys/strings in Uppy core constructor
  • allowing an option like formatError which is a function that gets the errror object and returns the i18nized string based on that.

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

No branches or pull requests

2 participants