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: bring back getResponseData #5354

Merged
merged 3 commits into from
Jul 18, 2024
Merged

Conversation

Murderlon
Copy link
Member

Closes #5349

getResponseData was deleted for 4.0 as I thought onAfterResponse covers the use case as well but you can't modify the response from inside it. I tried altering onAfterResponse to allow you to return a body and deprecate the void signature with backwards compatibility, but it didn't feel right and onAfterResponse is also called after every request, even it it fails. getResponseData is only called once after success(full retries).

@Murderlon Murderlon requested review from mifi and aduh95 July 17, 2024 15:47
@Murderlon Murderlon self-assigned this Jul 17, 2024
Copy link
Contributor

github-actions bot commented Jul 17, 2024

Diff output files
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index c731d7e..e429261 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -133,6 +133,7 @@ export default class XHRUpload extends BasePlugin {
     _classPrivateFieldLooseBase(this, _getFetcher)[_getFetcher] = files => {
       return async (url, options) => {
         try {
+          var _this$opts$getRespons, _this$opts, _body2;
           const res = await fetcher(url, {
             ...options,
             onBeforeRequest: this.opts.onBeforeRequest,
@@ -160,8 +161,21 @@ export default class XHRUpload extends BasePlugin {
               }
             },
           });
-          const body = JSON.parse(res.responseText);
-          const uploadURL = typeof (body == null ? void 0 : body.url) === "string" ? body.url : undefined;
+          let body = await ((_this$opts$getRespons = (_this$opts = this.opts).getResponseData) == null
+            ? void 0
+            : _this$opts$getRespons.call(_this$opts, res));
+          try {
+            var _body;
+            (_body = body) != null ? _body : body = JSON.parse(res.responseText);
+          } catch (cause) {
+            throw new Error(
+              "@uppy/xhr-upload expects a JSON response (with a `url` property). To parse non-JSON responses, use `getResponseData` to turn your response into JSON.",
+              {
+                cause,
+              },
+            );
+          }
+          const uploadURL = typeof ((_body2 = body) == null ? void 0 : _body2.url) === "string" ? body.url : undefined;
           for (const file of files) {
             this.uppy.emit("upload-success", file, {
               status: res.status,

const body = JSON.parse(res.responseText) as B
let body = await this.opts.getResponseData?.(res)
try {
body ??= JSON.parse(res.responseText) as B

Choose a reason for hiding this comment

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

Sorry for interfering.

This code overrides the value returned by getResponseData. For me, as a user, this behavior would be unexpected. The responseText might be a proper JSON document but with wrong field names that I would like to modify in getResponseData. I propose to check whether getResponseData is provided before parsing the response.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's not what happens here. Through optional chaining, getResponseData is only called if it exists, otherwise body is undefined. JSON.parse is only called if body is undefined (??=).

Choose a reason for hiding this comment

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

ah, overlooked this 😄 well done!

packages/@uppy/xhr-upload/src/index.ts Outdated Show resolved Hide resolved
@aduh95 aduh95 merged commit 7b92d9d into main Jul 18, 2024
17 checks passed
@aduh95 aduh95 deleted the xhr-upload-getResponseData branch July 18, 2024 14:22
github-actions bot added a commit that referenced this pull request Jul 18, 2024
| Package          | Version | Package          | Version |
| ---------------- | ------- | ---------------- | ------- |
| @uppy/aws-s3     |   4.0.1 | uppy             |   4.0.5 |
| @uppy/xhr-upload |   4.0.2 |                  |         |

- @uppy/aws-s3: use default `Body` generic & export `AwsBody` (Merlijn Vos / #5353)
- @uppy/xhr-upload: bring back getResponseData (Merlijn Vos / #5354)
- @uppy/aws-s3: only send `PartNumber` and `ETag` in completion request (Antoine du Hamel / #5356)
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.

XHR plugin expects upload response to be a valid JSON
3 participants