tus-js-client icon indicating copy to clipboard operation
tus-js-client copied to clipboard

originalRequest and originalResponse in DetailesError are stored as reference

Open AlexAndBear opened this issue 3 years ago • 3 comments

Describe the bug https://github.com/tus/tus-js-client/blob/14c3634e0ccd45973e90a3e460ca5e749f630107/lib/error.js#L5 reqand res will be stored as a reference in originalRequest and originalResponse.

This is quite cool, as we can access the response, get the status and do proper error handling in the client to show a human-readable error message.

However, using uppy with tus-js-client as plugin will call abort on the request in the error case, which will set the readyState but also the status to 0 (see: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/abort)

So I won't be able to read the status code in my underlying code.

Is it possible to store req and res as a deep copy, for proper error handling?

AlexAndBear avatar Aug 25 '22 13:08 AlexAndBear

Thank you for this report. I am a bit split on this topic. On the one hand, I see the reason for why you would not want to modify the error after it has been raised. On the other, I can imagine there are situations where you want to have a reference and use it for further diagnostic measures. In addition, making a deep copy might cause more confusion for users because that is not what they would expect.

However, using uppy with tus-js-client as plugin will call abort on the request in the error case, which will set the readyState but also the status to 0

Maybe it is possible to fix this inside Uppy, so that abort does not get called? I could not think of a specific need for this call after an error has been raised. Could you bring this issue to the Uppy repository?

Acconut avatar Sep 09 '22 21:09 Acconut

It looks like Uppy aborts tus when it encounters any error (could also be an error unrelated to Tus I think?) e.g. the onError option: https://github.com/transloadit/uppy/blob/952fb8dab3530bc04d4dc7694bd7d44f4aae8874/packages/%40uppy/tus/src/index.js#L262

Then it calls the cleanup function resetUploaderReferences which aborts the tus upload. If it were not to call this function, I think it could cause memory leaks and upload continuing in the background even after error.

Maybe if Tus had a boolean property like isRunning or hasFailed (I couldn't find any such property), then we could check that property first and only abort if tus has not failed, e.g. in resetUploaderReferences:

if (!uploader.hasFailed) uploader.abort();

What do you think?

mifi avatar Sep 12 '22 11:09 mifi

Maybe if Tus had a boolean property like isRunning or hasFailed (I couldn't find any such property), then we could check that property first and only abort if tus has not failed, e.g. in resetUploaderReferences:

In https://github.com/tus/tus-js-client/issues/453 we talked about exposing such properties after an internal modification of tus-js-client to use a state machine. So I hope to add such properties soon, but there is no ETA available right now.

Acconut avatar Sep 14 '22 22:09 Acconut