@uppy/aws-s3 response undefined while listening for "upload-error" event

[qxprakash]

Hi @Murderlon , this looked like an easy fix for #5672 but there are a few instances in the codebase where we're emitting "upload-error" without passing the response e.g.

uppy/packages/@uppy/transloadit/src/index.ts

Lines 833 to 837 in f877dac

	this.uppy.emit('preprocess-complete', file)
	this.uppy.emit('upload-error', file, err)
	})
	throw err
	}

I can see that response is an optional param in the interface , so then it seems like it's by design that response was not passed correct ? aren't we expecting users to see the response in case of upload-error event in these cases ?

not sure about this hence will keep it as a Draft PR for now :)

let me know if this works I'll add tests.

[github-actions]

Diff output files

diff --git a/packages/@uppy/aws-s3/lib/index.js b/packages/@uppy/aws-s3/lib/index.js
index b2bf3c6..3643c5b 100644
--- a/packages/@uppy/aws-s3/lib/index.js
+++ b/packages/@uppy/aws-s3/lib/index.js
@@ -623,8 +623,17 @@ function _uploadLocalFile2(file) {
       });
     };
     const onError = err => {
+      var _file$progress;
       this.uppy.log(err);
-      this.uppy.emit("upload-error", file, err);
+      const errorResponse = {
+        status: (err == null ? void 0 : err.status) || 500,
+        body: {
+          message: err instanceof Error ? err.message : "Unknown error",
+        },
+        bytesUploaded:
+          (file == null || (_file$progress = file.progress) == null ? void 0 : _file$progress.bytesUploaded) || 0,
+      };
+      this.uppy.emit("upload-error", file, err, errorResponse);
       this.resetUploaderReferences(file.id);
       reject(err);
     };

[Murderlon]

I don't think it makes sense to magically derive this from the error, it should be passed explicitly where we have access to the response (where onError is called, as the second argument)

However it seems we never emit a response anywhere so basically the types are lying. It would be good to have it consistent everywhere though.

[qxprakash]

I don't think it makes sense to magically derive this from the error, it should be passed explicitly where we have access to the response (where onError is called, as the second argument)

However it seems we never emit a response anywhere so basically the types are lying. It would be good to have it consistent everywhere though.

yeah ! just found one instance of 'upload-error' where response was passed

uppy/packages/@uppy/xhr-upload/src/index.ts

Lines 270 to 277 in f877dac

	this.uppy.emit(
	'upload-error',
	this.uppy.getFile(file.id),
	buildResponseError(request, error),
	request,
	)
	}

we're building the error response using buildResponseError using request and the error

so basically it means we can close #5672 as undefined was expected in response in case of 'upload-error' event ?

[Murderlon]

I think it would be nice if all uploaders did pass the actual request that failed to upload-error. The problem is that a generalized type doesn't make sense for that. So we would have to remove the response argument from the definition in @uppy/core and then inject the type into the namespace for each uploader, as is done here:

uppy/packages/@uppy/transloadit/src/index.ts

Lines 183 to 221 in 38333f7

	declare module '@uppy/core' {
	// eslint-disable-next-line @typescript-eslint/no-unused-vars
	export interface UppyEventMap<M extends Meta, B extends Body> {
	// We're also overriding the `restored` event as it is now populated with Transloadit state.
	restored: (pluginData: Record<string, TransloaditState>) => void
	'restore:get-data': (
	setData: (arg: Record<string, PersistentState>) => void,
	) => void
	'transloadit:assembly-created': (
	assembly: AssemblyResponse,
	fileIDs: string[],
	) => void
	'transloadit:assembly-cancel': (assembly: AssemblyResponse) => void
	'transloadit:import-error': (
	assembly: AssemblyResponse,
	fileID: string,
	error: Error,
	) => void
	'transloadit:assembly-error': (
	assembly: AssemblyResponse,
	error: Error,
	) => void
	'transloadit:assembly-executing': (assembly: AssemblyResponse) => void
	'transloadit:assembly-cancelled': (assembly: AssemblyResponse) => void
	'transloadit:upload': (
	file: AssemblyFile,
	assembly: AssemblyResponse,
	) => void
	'transloadit:result': (
	stepName: string,
	result: AssemblyResult,
	assembly: AssemblyResponse,
	) => void
	'transloadit:complete': (assembly: AssemblyResponse) => void
	'transloadit:execution-progress': (details: {
	progress_combined?: number
	}) => void
	}
	}

Also in the case of xhr-upload it passes XMLHttpRequest, which I'm surprised doesn't case a type error 🤔

It would require more digging to figure where the failed request is surfaced in each uploader and how to get it to 'upload-error'.

Up to you if you want to go deeper or do something else.