uppy.upload() does not work with XHRUploader

[bdirito]

Initial checklist

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

Link to runnable example

https://stackblitz.com/edit/vitejs-vite-6cja6cft?file=main.js,index.html

Steps to reproduce

Go to the linked stackblitz
Open console.
Add a single file to the dashboard to upload
Click the upload() button to invoke uppy.upload()
The upload is expected to fail (the POST url is bad)
Click the upload() button to invoke uppy.upload() again for the second time

Expected behavior

On both (or any) button press of upload() the following should happen:

• the browser should attempt to preform the POST
• the browser post should fail
• the upload() promise response should resolve and have one response.failed element
• the ui should transition to 'uploading', then transition to 'failed' from there

Actual behavior

On 'first' upload():

• Everything that is 'expected' happens except the promise does not resolve

On second 'second upload()':

• the promise resolves successfully; however both successful and failed are empty arrays
• no errors are printed to the log
• the browser makes no POST calls
• the UI transitions to an 'uploading' state that never ends

There are likely several things going on here but at least one aspect seems to be in https://github.com/transloadit/uppy/blob/main/packages/%40uppy/xhr-upload/src/index.ts starting on line 497

  #handleUpload = async (fileIDs: string[]) => {
    if (fileIDs.length === 0) {
      this.uppy.log('[XHRUpload] No files to upload!')
      return
    }
 
    [...]

    const filesFiltered = filterNonFailedFiles(files)
    const filesToEmit = filterFilesToEmitUploadStarted(filesFiltered)

Workaround:
I am using

    const currentFiles = uppy.getFiles();
    if (currentFiles.some((f) => f.error)) {
      await uppy.retryAll();
    } else {
     void uppy.upload();
    }

to alternatively call retryAll() instead of upload().

The ugliness here is that checking state in this way is based on digging into the internal uppy state; I do not know how I am 'supposed' to check what api call I should be making. The ideal situation is that there would be one api call that I could make in both cases. I also tried retryAll() as the 'do everything' api call but it does not work unless an upload() has been called first.

[qxprakash]

Hi @bdirito thanks for the detailed description of the issue , was able to succesfully reproduce it , just a small question , even after trying retryAll after clicking on upload() once, should retryAll() fail with resolved promise with failed file array , similar to what you're expecting to happen in upload() for the first time ?

[Murderlon]

Hi, I'm kind of okay with using retryAll when errors occurred and the uploads are done instead of upload (although upload shouldn't give an incorrect state) but I fully agree with this:

The ugliness here is that checking state in this way is based on digging into the internal uppy state; I do not know how I am 'supposed' to check what api call I should be making

This ties into a bigger problem of not being able to deterministically know which state Uppy is in. Instead of a quickfix, I'm thinking to turn Uppy into a finite state machine with states like idle | editing | preprocessing | uploading | postprocessing | failed | succeeded.

This doesn't have to be a major undertaking at first, it could be a new property on Uppy's state and just switching that internally.

[bdirito]

@qxprakash The docs do not actually say what the return value of retryAll() is supposed to be. https://uppy.io/docs/uppy/#retryall

Looking at the typescript defs (@uppy/core/src/Uppy.ts:L1374) gives us

retryAll(): Promise<UploadResult<M, B> | undefined> {

That's incidentally the same definition as upload() now that I look at it @uppy/core/src/Uppy.ts/L2244

upload(): Promise<NonNullable<UploadResult<M, B>> | undefined> {

Note that the upload()'s type definition does NOT match the docs in that the docs make no mention of a possible Promise<undefined> return.

If you are asking me what I think the returns of retryAll() (and upload()) 'should' be then I think I'd like to see Promise<UploadResult<M, B>>. Both apis would have the same return results; as retryAll() would just be an alias for upload().

• Every file in uppy.files() at the time the call was invoked must be accounted for in either the success or failure array.
• | undefined would be stripped as a possibility; I don't know what undefined means - does it mean 'everything failed'? What is the difference between undefined and a 'full' failure array?

Regardless of choice the important thing is documentation.

@Murderlon Even if you implement a 'getState' api call I think I would like to see 'stateless' upload() and retryAll(). retryAll() would basically just be an alias for upload(). I think this would be a better developer experience as it helps abstract away that state. Having said that making these apis 'stateless' has its own drawbacks too; the primary reasons I think 'stateless' would be better is that:

• The docs would have to explain the possible states, how to get that state information, and what apis work (or do not work) in certain states
• The apis would still have to handle all of that complexity anyway, even if to just throw an error. This would include state transitions in the middle of an async call (both upload() and retryAll() are async) so I'm not even convinced 'stateful' apis are that much simpler to write

Having said that, having stateful api calls such as upload() and retryAll() is a potentially valid design. The current API docs don't really explain a lot of these details so its hard to know what the 'intended' approach to a lot of this stuff is.

[bdirito]

@qxprakash I was going a bit too fast; upload()'s response is specifically a NonNullable form of UploadResponse (if returning an UploadResponse at all). retryAll() however does NOT have that NonNullable;

export interface UploadResult<M extends Meta, B extends Body> {
  successful?: UppyFile<M, B>[]
  failed?: UppyFile<M, B>[]
  uploadID?: string
  [key: string]: unknown
}

Again I would prefer if the two apis worked and behaved the 'same way' meaning that retryAll() would also return a NonNullable form of UploadResult. Though since its not in the docs the existing typescript lets retryAll() return quite a bit more variants then it actually does

[qxprakash]

Hi @bdirito I got this working with a few workarounds , I don't think my solution is sustainable but atleast I got to to learn a lot about upyy's state management

Here's a video showcasing the exact expected outcome which you described in the issue, I was trying to host this stackblitz but ran into few errors with yarn there while setting that up
so only a video for now and I will push the code in my fork if you want to check out

uppy_bug_fix.mp4

So let's start with the first issue which was

On 'first' upload():

Everything that is 'expected' happens except the promise does not resolve

now this was due to couple of reasons

firstly

• when it emits upload-error event

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

  Lines 270 to 278 in f877dac

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

• the event listener for this event in core/uppy.ts listens for it and calls this.#informAndEmit([newError]) with the newError object (line 1625 below)

uppy/packages/@uppy/core/src/Uppy.ts

Lines 1612 to 1629 in f877dac

	this.on('upload-error', (file, error, response) => {
	errorHandler(error, file, response)
	if (typeof error === 'object' && error.message) {
	this.log(error.message, 'error')
	const newError = new Error(
	this.i18n('failedToUpload', { file: file?.name ?? '' }),
	) as any // we may want a new custom error here
	newError.isUserFacing = true // todo maybe don't do this with all errors?
	newError.details = error.message
	if (error.details) {
	newError.details += ` ${error.details}`
	}
	this.#informAndEmit([newError])
	} else {
	this.#informAndEmit([error])
	}
	})

• but it didn't attach the param file into the newError object which if you see the this.#informAndEmit([newError]) function below it expects a file (line 852 below) and emits another event error
  with this.emit('error', error, error.file) where it's passing the file using error.file (line 863 below)

uppy/packages/@uppy/core/src/Uppy.ts

Lines 845 to 866 in f877dac

	#informAndEmit(
	errors: {
	name: string
	message: string
	isUserFacing?: boolean
	details?: string
	isRestriction?: boolean
	file?: UppyFile<M, B>
	}[],
	): void {
	for (const error of errors) {
	if (error.isRestriction) {
	this.emit(
	'restriction-failed',
	error.file,
	error as RestrictionError<M, B>,
	)
	} else {
	this.emit('error', error, error.file)
	}
	this.log(error, 'warning')
	}

• see the error event listener , and on line 1603 this is where the file state is being set to error

uppy/packages/@uppy/core/src/Uppy.ts

Lines 1588 to 1610 in f877dac

	#addListeners(): void {
	// Type inference only works for inline functions so we have to type it again
	const errorHandler: UppyEventMap<M, B>['error'] = (
	error,
	file,
	response,
	) => {
	let errorMsg = error.message || 'Unknown error'
	if (error.details) {
	errorMsg += ` ${error.details}`
	}
	this.setState({ error: errorMsg })
	if (file != null && file.id in this.getState().files) {
	this.setFileState(file.id, {
	error: errorMsg,
	response,
	})
	}
	}
	this.on('error', errorHandler)

so TLDR adding file to newError object before passing it to this.#informAndEmit([newError]) would have fixed the problem right ? but then I saw another thing which was kindof blocking this and it was this

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

Lines 263 to 282 in f877dac

	} catch (error) {
	if (error.name === 'AbortError') {
	return undefined
	}
	const request = error.request as XMLHttpRequest | undefined
	for (const file of files) {
	this.uppy.emit(
	'upload-error',
	this.uppy.getFile(file.id),
	buildResponseError(request, error),
	request,
	)
	}
	throw error
	}
	}
	}
	}

you can see here after catching the error and emitting the 'upload-error' event , it's again throwing error again on line 278 which was triggering 'error' event handler in core/uppy.ts and since it only threw the error without any file or response as required by the 'error' event handler it ends abrupty with rejected promise , so I have to comment the re-throwing of error and after that it worked

open to feedback about this feel free to correct me if I'm missing anything here. @Murderlon @bdirito

@Murderlon I will submit a draft PR for this where we can continue our discussion furthur

now moving on to the second issue.

[qxprakash]

Second Issue :

In second 'second upload()':

• the promise resolves successfully; however both successful and failed are empty arrays
• no errors are printed to the log
• the browser makes no POST calls
• the UI transitions to an 'uploading' state that never ends

It is linked to how uppy considers the state of files with failed uploads ,

after clicking on upload first time , when it fails and promise gets resolved with failed file state

this is what it looks like

    "source": "Dashboard",
    "id": "uppy-vitejs/vite/6cja6cft/zip-1d-1d-1e-application/zip-62911-1741028447758",
    "name": "vitejs-vite-6cja6cft.zip",
    "extension": "zip",
    "meta": {
        "relativePath": null,
        "name": "vitejs-vite-6cja6cft.zip",
        "type": "application/zip"
    },
    "type": "application/zip",
    "data": {},
    "progress": {
        "uploadStarted": 1741104085263,
        "uploadComplete": false,
        "bytesUploaded": 62911,
        "bytesTotal": 62911,
        "percentage": 100
    },
    "size": 62911,
    "isGhost": false,
    "isRemote": false,
    "error": "Failed to upload vitejs-vite-6cja6cft.zip This looks like a network error, the endpoint might be blocked by an internet provider or a firewall."
}

and when you click on upload again it doesn't get's added to waitingFileIDs array
because it fails the below condition , because in uppy's state file's upload already started

uppy/packages/@uppy/core/src/Uppy.ts

Lines 2298 to 2304 in f877dac

	if (
	!file.progress.uploadStarted &&
	currentlyUploadingFiles.indexOf(fileID) === -1
	) {
	waitingFileIDs.push(file.id)
	}
	})

due to this

this.#createUpload(waitingFileIDs) get's called with empty waitingFileIDs array

and eventually #handleUpload get called with empty array of fileIDs

and returns because there are no files to upload and the loader keeps on spinning

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

Lines 497 to 501 in f877dac

	#handleUpload = async (fileIDs: string[]) => {
	if (fileIDs.length === 0) {
	this.uppy.log('[XHRUpload] No files to upload!')
	return
	}

for this to work ideally as you described in the issue , after the upload gets failed the first time , the selected file should be treated like as a new file (progress state has to be reset) , upload should start and fail with resolved promise and failed file arrays, that's the workaround I tried to get this work , I reset the progress and error , but I think this violates how uppy considers the state of the files , that's why I got few failing tests :( , would be happy to discuss on this , there are quite a few ways we can take to fix this.