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: do not throw when res is missing url #5132

Merged
merged 1 commit into from
May 2, 2024

Conversation

Murderlon
Copy link
Member

Closes #5131

Accidentally introduced a breaking change. I thought sending back a url is mandatory and that's why we have things like getResponseData and responseUrlFieldName so you can get it in a flexible way. But it seems it wasn't needed and people didn't do it and now their uploads stopped working.

@Murderlon Murderlon requested a review from aduh95 May 1, 2024 08:32
@Murderlon Murderlon self-assigned this May 1, 2024
Copy link
Contributor

github-actions bot commented May 1, 2024

Diff output files
diff --git a/packages/@uppy/xhr-upload/lib/index.js b/packages/@uppy/xhr-upload/lib/index.js
index cf9da5c..b70a498 100644
--- a/packages/@uppy/xhr-upload/lib/index.js
+++ b/packages/@uppy/xhr-upload/lib/index.js
@@ -177,12 +177,7 @@ export default class XHRUpload extends BasePlugin {
             throw new NetworkError(res.statusText, res);
           }
           const body = this.opts.getResponseData(res.responseText, res);
-          const uploadURL = body[this.opts.responseUrlFieldName];
-          if (typeof uploadURL !== "string") {
-            throw new Error(
-              `The received response did not include a valid URL for key ${this.opts.responseUrlFieldName}`,
-            );
-          }
+          const uploadURL = body == null ? void 0 : body[this.opts.responseUrlFieldName];
           for (const file of files) {
             this.uppy.emit("upload-success", file, {
               status: res.status,

Comment on lines +242 to +244
const uploadURL = body?.[this.opts.responseUrlFieldName] as
| string
| undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const uploadURL = body?.[this.opts.responseUrlFieldName] as
| string
| undefined
const uploadURL = body[this.opts.responseUrlFieldName]
if (!(typeof uploadURL === 'string' || uploadURL == null)) {
throw new Error(
`The received response did not include a valid URL for option responseUrlFieldName ${this.opts.responseUrlFieldName}`,
)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

As you can read in the issue and my comment, the whole point is that this should not be required.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, and i think my suggested code is a safer way to check that rather than using a blind type assertion

Copy link
Member Author

Choose a reason for hiding this comment

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

This PR removes the error, an undefined url is allowed. It was a breaking change to throw an error. You suggestion makes the exact same again as before this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

i'm not sure how my suggestion would make it the exact same as before this PR, as i added a check that allows undefined and null (== operator).

If uploadURL is undefined, !(typeof uploadURL === 'string' || uploadURL == null) evaluates to false, hence it will not throw the error.

@Murderlon Murderlon merged commit 2a15ba5 into main May 2, 2024
17 checks passed
@Murderlon Murderlon deleted the xhr-no-url-throw branch May 2, 2024 08:20
Murderlon added a commit that referenced this pull request May 2, 2024
* main:
  meta: enable prettier for markdown (#5133)
  @uppy/xhr-upload: do not throw when res is missing url (#5132)
  @uppy/companion: coerce `requestUrl` to a string (#5128)
  Release: uppy@3.25.0 (#5127)
  meta: enforce use of `.js` extension in `import type` declarations (#5126)
  @uppy/core: add instance ID to generated IDs (#5080)
  @uppy/core: reference updated i18n in Restricter (#5118)
Murderlon added a commit that referenced this pull request May 2, 2024
* 4.x:
  meta: enable prettier for markdown (#5133)
  @uppy/xhr-upload: do not throw when res is missing url (#5132)
  Release: uppy@4.0.0-beta.4 (#5130)
  meta: fix release script
  Upgrade Yarn to 4.x (#4849)
  @uppy/companion: coerce `requestUrl` to a string (#5128)
  Release: uppy@3.25.0 (#5127)
  meta: enforce use of `.js` extension in `import type` declarations (#5126)
  @uppy/core: add instance ID to generated IDs (#5080)
  @uppy/core: reference updated i18n in Restricter (#5118)
@github-actions github-actions bot mentioned this pull request May 3, 2024
github-actions bot added a commit that referenced this pull request May 3, 2024
| Package          | Version | Package          | Version |
| ---------------- | ------- | ---------------- | ------- |
| @uppy/companion  |  4.13.2 | @uppy/xhr-upload |   3.6.6 |
| @uppy/core       |  3.11.1 | uppy             |  3.25.1 |
| @uppy/locales    |   3.5.3 |                  |         |

- @uppy/locales: Update ru_RU locale  (Uladzislau Bodryi / #5120)
- meta: fix `update-contributors` script (Antoine du Hamel / #5137)
- meta: fix `bullet` setting for ReMark (Antoine du Hamel)
- meta: add prettier to `.md` pre-commit hooks (Antoine du Hamel)
- @uppy/core: make UppyEventMap more readable (Murderlon)
- meta: enable prettier for markdown (Merlijn Vos / #5133)
- @uppy/xhr-upload: do not throw when res is missing url (Merlijn Vos / #5132)
- @uppy/companion: coerce `requestUrl` to a string (Antoine du Hamel / #5128)
github-actions bot added a commit that referenced this pull request May 3, 2024
| Package                   |      Version | Package                   |      Version |
| ------------------------- | ------------ | ------------------------- | ------------ |
| @uppy/audio               | 2.0.0-beta.5 | @uppy/progress-bar        | 4.0.0-beta.4 |
| @uppy/aws-s3              | 4.0.0-beta.3 | @uppy/provider-views      | 4.0.0-beta.5 |
| @uppy/aws-s3-multipart    | 4.0.0-beta.5 | @uppy/react               | 4.0.0-beta.5 |
| @uppy/box                 | 3.0.0-beta.5 | @uppy/redux-dev-tools     | 4.0.0-beta.2 |
| @uppy/companion           | 5.0.0-beta.5 | @uppy/remote-sources      | 2.0.0-beta.4 |
| @uppy/companion-client    | 4.0.0-beta.5 | @uppy/screen-capture      | 4.0.0-beta.4 |
| @uppy/compressor          | 2.0.0-beta.5 | @uppy/status-bar          | 4.0.0-beta.5 |
| @uppy/core                | 4.0.0-beta.5 | @uppy/store-default       | 4.0.0-beta.2 |
| @uppy/dashboard           | 4.0.0-beta.5 | @uppy/store-redux         | 4.0.0-beta.3 |
| @uppy/drag-drop           | 4.0.0-beta.4 | @uppy/svelte              | 4.0.0-beta.3 |
| @uppy/dropbox             | 4.0.0-beta.5 | @uppy/thumbnail-generator | 4.0.0-beta.4 |
| @uppy/facebook            | 4.0.0-beta.5 | @uppy/transloadit         | 4.0.0-beta.5 |
| @uppy/file-input          | 4.0.0-beta.5 | @uppy/tus                 | 4.0.0-beta.4 |
| @uppy/form                | 4.0.0-beta.3 | @uppy/unsplash            | 4.0.0-beta.5 |
| @uppy/golden-retriever    | 4.0.0-beta.4 | @uppy/url                 | 4.0.0-beta.5 |
| @uppy/google-drive        | 4.0.0-beta.5 | @uppy/utils               | 6.0.0-beta.5 |
| @uppy/image-editor        | 3.0.0-beta.3 | @uppy/vue                 | 2.0.0-beta.2 |
| @uppy/informer            | 4.0.0-beta.2 | @uppy/webcam              | 4.0.0-beta.5 |
| @uppy/instagram           | 4.0.0-beta.5 | @uppy/xhr-upload          | 4.0.0-beta.3 |
| @uppy/locales             | 4.0.0-beta.1 | @uppy/zoom                | 3.0.0-beta.5 |
| @uppy/onedrive            | 4.0.0-beta.5 | uppy                      | 4.0.0-beta.5 |

- @uppy/core: make UppyEventMap more readable (Murderlon)
- @uppy/audio,@uppy/compressor,@uppy/core,@uppy/dashboard,@uppy/remote-sources,@uppy/tus,@uppy/utils: Format (Murderlon)
- @uppy/aws-s3-multipart: Format (Murderlon)
- meta: enable prettier for markdown (Merlijn Vos / #5133)
- @uppy/xhr-upload: do not throw when res is missing url (Merlijn Vos / #5132)
- @uppy/companion: coerce `requestUrl` to a string (Antoine du Hamel / #5128)
- meta: enforce use of `.js` extension in `import type` declarations (Antoine du Hamel / #5126)
- @uppy/core: add instance ID to generated IDs (Merlijn Vos / #5080)
- @uppy/core: reference updated i18n in Restricter (Merlijn Vos / #5118)
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.

XHRUpload stuck at 100% despite 2xx response code.
3 participants