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

Uppy companion fails silently very often #4355

Closed
2 tasks done
Johnrobmiller opened this issue Mar 15, 2023 · 6 comments
Closed
2 tasks done

Uppy companion fails silently very often #4355

Johnrobmiller opened this issue Mar 15, 2023 · 6 comments
Labels

Comments

@Johnrobmiller
Copy link

Initial checklist

  • I understand this is a feature request and questions should be posted in the Community Forum
  • I searched issues and couldn’t find anything (or linked relevant results below)

Problem

I mean this in the nicest way possible, but the Uppy companion fails silently more than any other library I've seen.

For example, in my COMPANION_HOST env variable, I put a / at the end, which caused it to fail silently. Then, there was also an https:// at the beginning, which caused it to fail silently. Before that, the uploadUrls were wrong, and it failed silently. Another time, I looked at the network tab to see why things were broken, and there was a CORS error, but that was actually just misleadingly caused by some other random-seeming option being badly formatted.

Solution

With some refactoring, the library could go from being in the top 1% of libraries that fail silently to never failing silently ever. It might even as simple as putting in guard rails, adding more try/catch blocks, or adding more descripting network responses. At the bare minimum, you should at least strongly type the companion options, which are currently typed as object.

Alternatives


@mifi
Copy link
Contributor

mifi commented Mar 15, 2023

Hi! Thanks for your feedback. By "fail silently", you mean that:

  1. the Uppy client does not log anything to console.log/console.error etc in the browser when something goes wrong?
  2. the Uppy client does not give the user any error message toast or popup when something goes wrong?
  3. Companion does not log anything to stdout when something is misconfigured?

@Johnrobmiller
Copy link
Author

Johnrobmiller commented Mar 15, 2023

Hi! Thanks for your feedback. By "fail silently", you mean that:

  1. the Uppy client does not log anything to console.log/console.error etc in the browser when something goes wrong?
  2. the Uppy client does not give the user any error message toast or popup when something goes wrong?
  3. Companion does not log anything to stdout when something is misconfigured?
  1. Yes, and also in the Node.js terminal
  2. If the error pertains to connecting to the companion, it does give a toast message on the front end, but the error message conveys no information. It might say something like "GET [endpoint] failed". I guess that's better than nothing at all, but it's so little information to work with that it's practically as bad as faililng with no error message. A lot of the times it failed silently were were after connecting to uppy, in which case it was totally silent.
  3. Sometimes companions logs to stdout, and other times it just fails silently, like in the examples above. I can provide more examples if you'd like.

@mifi
Copy link
Contributor

mifi commented Mar 17, 2023

by COMPANION_HOST, I assume you mean COMPANION_PATH. For me, when I set COMPANION_PATH=/subpath/ or COMPANION_PATH=http://127.0.0.1:5173 I get errors in my JS console:

fetchWithNetworkError.js:7          GET http://localhost:3020/drive/list/root net::ERR_FAILED
 [Uppy] [11:41:11] Error: Could not GET http://localhost:3020/drive/list/root

And the user also receives and error about failing to connect to the provider. So not sure how to reproduce this.

If you're able to reproduce and know how to fix it, feel free to submit a PR :)

I tried to set COMPANION_UPLOAD_URLS=http://google.com and run an upload, and it does not fail silently. It logs both an error in companion and show to the user that the upload failed:

companion: 2023-03-17T03:51:14.656Z [debug] uploader.validator.fail upload destination does not match any allowed destinations

Screenshot 2023-03-17 at 11 51 20

  • If the error pertains to connecting to the companion, it does give a toast message on the front end, but the error message conveys no information. It might say something like "GET [endpoint] failed". I guess that's better than nothing at all, but it's so little information to work with that it's practically as bad as faililng with no error message. A lot of the times it failed silently were were after connecting to uppy, in which case it was totally silent.

I'm not sure if I agree with this. I think user-facing error messages should not contain technical mumbo-jumbo that the user does not understand. This information should instead be logged and picked up by a crash reporting system by the developers of the app and then acted upon. In development, logging to console.error is a good way for developers to identify problems, and the error doesn't need to be presented to them in a toast.

However if the toast error message is not very good, maybe we could improve it, like instead saying "Please try again or contact us if the problem persists"

3. Sometimes companions logs to stdout, and other times it just fails silently, like in the examples above. I can provide more examples if you'd like.

Yes, please provide reproducible examples on how to make companion fail silently, that would be very valuable for fixing these problems.

I agree that we can improve our typescript definitions. I've created a master issue for this: #4361

@Johnrobmiller
Copy link
Author

Johnrobmiller commented Mar 17, 2023

Regarding the COMPANION_HOST error, my apologies for not being clear enough about that. What I meant to say was this.

Given these URLs:

my-uppy-companion.com <-- (this works)
my-uppy-companion.com/ <-- (this fails with an unhelpful error message)
https://my-uppy-companion.com <-- (this fails with an unhelpful error message)

I feel like it's possible to use regex under the hood to auto-format all URLs to match the required formatting. However, if that is not possible, then a nicer error message that clearly tells the user what the issue is would save some time.

My original point is that there are a lot of moments when I set up companion where there is either

  • no error message at all
  • a nondescript error message (e.g., saying that a GET failed without any information as to why it failed)
  • a misleading error message (i.e., the error messages in the network tab saying there was a CORS issue when, in reality, is was just another formatting error in the options for uppy companion)

@mifi
Copy link
Contributor

mifi commented Mar 18, 2023

I'm stil lnot sure where you're putting those URLs you described, in order to trigger a silent failure. I cannot find any COMPANION_HOST variable in Companion. Could you explain how you reproduced this problem?

My original point is that there are a lot of moments when I set up companion where there is either

  • no error message at all
  • a nondescript error message (e.g., saying that a GET failed without any information as to why it failed)
  • a misleading error message (i.e., the error messages in the network tab saying there was a CORS issue when, in reality, is was just another formatting error in the options for uppy companion)

Can you provide an example of how to reproduce these?

Unfortunately CORS errors are known to be cryptic, for security reasons and the way they are implemented in the browsers, and not so much we can do about that, I believe.

@mifi
Copy link
Contributor

mifi commented Mar 20, 2023

closing this until we have some reproducible code

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