muxinc / upchunk

Uploads Chunks! Takes big files, splits them up, then uploads each one with care (and PUT requests).
MIT License
329 stars 46 forks source link

Failed chunk upload callback response is undefined error #142

Open luis-tan opened 1 month ago

luis-tan commented 1 month ago

If a chunk upload fails and the failed chunk upload callback is triggered, the preceding check accounts for the response potentially being undefined, but the callback itself does not.

This actually caused an issue in our production code where we couldn't even notify the user an error had ocurred because the "error" dispatch event never happened.

Here's the offending line: https://github.com/muxinc/upchunk/blob/master/src/upchunk.ts#L749

I'm happy to open a PR, but couldn't find a contributions document. The message could simply be ammended to something along the lines of:

      const message = res ? `Server responded with ${res.statusCode}. Stopping upload.` : 'Unable to connect with server. Stopping upload.';

      this.dispatch('error', {
        message,
        chunk: this.chunkCount,
        attempts: this.attemptCount,
        response: res,
      });

It's unclear what caused the error in the first place, but at least this way I can show a message to the user.

cjpillsbury commented 1 month ago

@luis-tan thanks for the issue! Yeah looks like some of our types are lying as well. Happy to accept a PR. If you could:

  1. make the changes you propose (your msg looks fine to me)
  2. update isFailedChunkUpload to remove the res is XhrResponseLike (that's the part that's lying)
  3. update failedChunkUploadCb signature so res: XhrResponseLike | undefined (so it's no longer lying with our more accurate type signature from (2))

Ideally we'd add some tests for this case as well in upchunk.spec.ts, but it sounds like you haven't quite pinned down the root cause, so I won't treat that as a blocker for your PR, but if you could chew on that a bit, that would be appreciated! Also happy to help/available for you to bounce ideas off of.