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

@uppy/xhr-upload: fix getResponseData regression #4964

Merged
merged 1 commit into from Feb 27, 2024

Conversation

Murderlon
Copy link
Member

Fixes #4963

@Murderlon Murderlon self-assigned this Feb 26, 2024
Copy link
Contributor

Diff output files
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index c9d4e85..4c45cea 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -53,7 +53,11 @@ const defaultOptions = {
   withCredentials: false,
   responseType: "",
   getResponseData(responseText) {
-    return JSON.parse(responseText);
+    let parsedResponse = {};
+    try {
+      parsedResponse = JSON.parse(responseText);
+    } catch {}
+    return parsedResponse;
   },
   getResponseError(_, response) {
     let error = new Error("Upload error");

Comment on lines +117 to +126
let parsedResponse = {}
try {
parsedResponse = JSON.parse(responseText)
} catch {
// ignore
}
// We don't have access to the B (Body) generic here
// so we have to cast it to any. The user facing types
// remain correct, this is only to please the merging of default options.
return parsedResponse as any
Copy link
Member

@aduh95 aduh95 Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think an empty object is a correct response here, undefined would be more appropriate, no?

Suggested change
let parsedResponse = {}
try {
parsedResponse = JSON.parse(responseText)
} catch {
// ignore
}
// We don't have access to the B (Body) generic here
// so we have to cast it to any. The user facing types
// remain correct, this is only to please the merging of default options.
return parsedResponse as any
try {
return JSON.parse(responseText)
} catch {
// ignore
}

or

Suggested change
let parsedResponse = {}
try {
parsedResponse = JSON.parse(responseText)
} catch {
// ignore
}
// We don't have access to the B (Body) generic here
// so we have to cast it to any. The user facing types
// remain correct, this is only to please the merging of default options.
return parsedResponse as any
if (responseText) return JSON.parse(responseText)

Copy link
Member Author

@Murderlon Murderlon Feb 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a regression because it used to return {}, until I accidentally removed it in #4892. So {} is what is has to be for now.

@aduh95 aduh95 merged commit b46652d into main Feb 27, 2024
16 checks passed
@aduh95 aduh95 deleted the getResponseData-regression branch February 27, 2024 17:06
@github-actions github-actions bot mentioned this pull request Feb 28, 2024
github-actions bot added a commit that referenced this pull request Feb 28, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/box              |   2.2.1 | @uppy/onedrive         |   3.2.1 |
| @uppy/companion-client |   3.7.4 | @uppy/progress-bar     |   3.1.0 |
| @uppy/core             |   3.9.3 | @uppy/provider-views   |  3.10.0 |
| @uppy/dashboard        |   3.7.5 | @uppy/status-bar       |   3.3.0 |
| @uppy/file-input       |   3.1.0 | @uppy/utils            |   5.7.4 |
| @uppy/form             |   3.2.0 | @uppy/xhr-upload       |   3.6.4 |
| @uppy/image-editor     |   2.4.4 | uppy                   |  3.23.0 |
| @uppy/informer         |   3.1.0 |                        |         |

- @uppy/form: migrate to TS (Merlijn Vos / #4937)
- @uppy/box: fetchPreAuthToken in box too (Mikael Finstad / #4969)
- @uppy/progress-bar: refactor to TypeScript (Mikael Finstad / #4921)
- @uppy/onedrive: fix custom oauth2 credentials for onedrive (Mikael Finstad / #4968)
- @uppy/companion-client,@uppy/utils,@uppy/xhr-upload: improvements for #4922 (Mikael Finstad / #4960)
- @uppy/utils: fix various type issues (Mikael Finstad / #4958)
- @uppy/provider-views: migrate to TS (Merlijn Vos / #4919)
- @uppy/utils: simplify `findDOMElements` (Mikael Finstad / #4957)
- @uppy/xhr-upload: fix getResponseData regression (Merlijn Vos / #4964)
- @uppy/informer: migrate to TS (Merlijn Vos / #4967)
- @uppy/core: remove unused import (Antoine du Hamel / #4972)
- @uppy/image-editor: remove default target (Merlijn Vos / #4966)
- @uppy/angular: Build fixes (Mikael Finstad / #4959)
- meta: Fix flaky e2e test (Murderlon)
- meta: fix e2e flake (Mikael Finstad / #4961)
- meta: add support for `Fragment` short syntax (Antoine du Hamel / #4953)
- @uppy/file-input: refactor to TypeScript (Antoine du Hamel / #4954)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@uppy/xhr-upload: upload fails if server returns 204 empty
2 participants