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

XHR plugin expects upload response to be a valid JSON #5349

Closed
2 tasks done
aonnikov opened this issue Jul 16, 2024 · 8 comments · Fixed by #5354
Closed
2 tasks done

XHR plugin expects upload response to be a valid JSON #5349

aonnikov opened this issue Jul 16, 2024 · 8 comments · Fixed by #5354
Assignees
Labels

Comments

@aonnikov
Copy link

aonnikov commented Jul 16, 2024

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

No response

Steps to reproduce

XHR plugin in Uppy 4.0 seem to require the file upload endpoint to return a valid JSON. There is a code that attempts to extract upload URL from the response:

          const body = JSON.parse(res.responseText) as B
          const uploadURL = typeof body?.url === 'string' ? body.url : undefined

If the response is not a JSON document, then upload fails.

Expected behavior

It is expected that this behavior is configurable or the code has proper error handling.

Actual behavior

File upload fails. I figured out that responseType option not used. This might be a regression introduced after migrating to fetcher use.

@aonnikov aonnikov added the Bug label Jul 16, 2024
@jaswindersingh1903
Copy link

Do you think this is the same issue I have
#5328

@aonnikov
Copy link
Author

Nope, to me this does not look like the same issue.

@Murderlon
Copy link
Member

Murderlon commented Jul 17, 2024

Hi, what do you want to return that is not JSON? We made it required because Uppy (ideally) needs to know where the resource that was just uploaded is located.

If you return XML, you could use onAfterResponse to return JSON with a url property.

@aonnikov
Copy link
Author

My particular service returns plain text with id of the uploaded file and I have no intention to change the response only to make Uppy happy. With 3.x it works just fine because there is the getResponseData that allows to parse response in any format and return body. In 4.0 there is no way to do this, because Uppy assumes response to be a JSON document; onAfterResponse returns void.

@Murderlon
Copy link
Member

I can make a change to allow onAfterResponse to return a different body

@Murderlon Murderlon self-assigned this Jul 17, 2024
@aonnikov
Copy link
Author

As I understand, upload location URL is needed for Golden Retriever, that makes sense ...
Is that possible to keep the same behavior as in Uppy 3.x? I.e. if getResponseData is provided, it is used to parse the response, otherwise Uppy attempts to parse it as a JSON document. Proper error handling is important here as well, it took me some time to realize that upload failed due to unsupported type of content returned by XHR upload service.

@Murderlon
Copy link
Member

I will log a better error but getResponseData has been removed and ideally we don't bring it back. The problem with the previous hooks was is that people want to do all kinds of things and they were overly specific. But we can use onAfterResponse to cover your use case with some changes.

@aonnikov
Copy link
Author

I'm new here and was not aware of problems users or devs run into with previous hooks. I started with 3.x version and was happy to see 4.x with improved typescript support, but the problem with XHR uploader forced me to stay on 3.x at least for now. From my point of view, requirement to return a document in a specific format is very strict limitation, and makes the XHR uploader very specific, whereas it is supposed to be a general purpose uploader. Users may not want or may not have an ability to adjust response format of their upload endpoints.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants