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/companion-client: fix body/url on upload-success #4922

Merged
merged 4 commits into from
Feb 22, 2024

Conversation

Murderlon
Copy link
Member

@Murderlon Murderlon commented Feb 15, 2024

Fixes #4915
Fixes #4909

Not the cleanest fix, but not sure how do it otherwise. Couple of problems:

  • Using tus/transloadit only returns a url back from Companion
  • Using xhr-upload also returns response and bytesUploaded. Inside response there is responseText, status, headers.
  • xhr-upload has options getResponseData, getResponseError, and responseUrlFieldName to extract data for local files, these options are not used for responses from Companion as they are handled inside companion-client. Those options where introduced for people who want to convert XML to JSON (can't believe we support that 🙈). Funnily enough, those people with XML were never able to use Companion as Companion expects the response to be JSON before sending it over websockets.
  • It's impossible to type this correctly for upload-success because companion-client has no knowledge of that. Nor can we type it properly internally.

So since these options we don't assume the response and where to find the upload URL, you can extract it / point to it. How do we do it in for Companion uploads then? Well we either guess on the client of guess inside companion. Currently Companion never sends the upload URL back:

return {
url: null,
extraData: { response: getRespObj(response), bytesUploaded },
}

But it does for tus/S3. Since Companion is responsible for sending back the upload url for other uploaders I decided to guess inside Companion rather than on the client.


What's the ideal fix? In 4.0 I think we should remove all these extraction options for xhr-upload and force people to return JSON with a url property. Realistically, if you run your own server you need to create a new endpoint for Uppy anyway, so why make your life difficult with XML? Or by returning a key different from url? I think we can easily enforce that.

Then ideally upload-success returns consistently, so not all kinds of different data for different uploaders. Not sure how that's possible without a deep dive.

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

github-actions bot commented Feb 15, 2024

Diff output files
diff --git a/packages/@uppy/companion-client/lib/RequestClient.js b/packages/@uppy/companion-client/lib/RequestClient.js
index 8277e2f..2687682 100644
--- a/packages/@uppy/companion-client/lib/RequestClient.js
+++ b/packages/@uppy/companion-client/lib/RequestClient.js
@@ -376,9 +376,18 @@ async function _awaitRemoteFileUpload2(_ref4) {
                         break;
                       }
                       case "success": {
-                        var _socketAbortControlle2;
+                        var _payload$response, _payload$response$sta, _payload$response2, _socketAbortControlle2;
+                        const text = (_payload$response = payload.response) == null
+                          ? void 0
+                          : _payload$response.responseText;
                         this.uppy.emit("upload-success", this.uppy.getFile(file.id), {
                           uploadURL: payload.url,
+                          status: (_payload$response$sta = (_payload$response2 = payload.response) == null
+                              ? void 0
+                              : _payload$response2.status) != null
+                            ? _payload$response$sta
+                            : 200,
+                          body: text ? JSON.parse(text) : undefined,
                         });
                         (_socketAbortControlle2 = socketAbortController) == null || _socketAbortControlle2.abort == null
                           || _socketAbortControlle2.abort();

@orban
Copy link

orban commented Feb 21, 2024

Let us know how we can support. Would love to get this merged as its currently a blocker for us.

@Murderlon
Copy link
Member Author

I think we'll have it released before Friday :)

* main:
  meta: disable `@typescript-eslint/no-non-null-assertion` lint rule (#4945)
  remove unnecessary `'use strict'` directives (#4943)
  @uppy/companion-client: type changes for provider-views (#4938)
  meta: bump ip from 1.1.8 to 1.1.9 (#4941)
  @uppy/companion-client: update types (#4927)
  Release: uppy@3.22.1 (#4935)
  update vi_VN translation (#4930)
  bump `@transloadit/prettier-bytes` (#4933)
  Release: uppy@3.22.0 (#4929)
  update `UppyFile` objects before emitting events (#4928)
  @uppy/transloadit: add `clientName` option (#4920)
  Fix broken previews after cropping (#4926)
  @uppy/compressor: upgrade compressorjs (#4924)
  Fix companion dns and allow redirects from http->https again (#4895)
  autoOpenFileEditor - rename "Edit file" to "Edit image" (#4925)
  meta: resolve jsx to Preact in shared tsconfig (#4923)
@Murderlon Murderlon merged commit 3d8ff79 into main Feb 22, 2024
19 checks passed
@Murderlon Murderlon deleted the requestclient-upload-success branch February 22, 2024 10:44
Murderlon added a commit that referenced this pull request Feb 22, 2024
* main:
  Introduce `ValidateableFile` & move `MinimalRequiredUppyFile` into utils (#4944)
  uppy: fix bundle builder (#4950)
  @uppy/core: improve `UIPluginOptions` types (#4946)
  @uppy/companion-client: fix body/url on upload-success (#4922)
  @uppy/utils: remove EventManager circular reference (#4949)
@github-actions github-actions bot mentioned this pull request Feb 22, 2024
github-actions bot added a commit that referenced this pull request Feb 22, 2024
| Package                | Version | Package                | Version |
| ---------------------- | ------- | ---------------------- | ------- |
| @uppy/audio            |   1.1.7 | @uppy/react            |   3.2.2 |
| @uppy/companion        |  4.12.3 | @uppy/status-bar       |   3.2.8 |
| @uppy/companion-client |   3.7.3 | @uppy/tus              |   3.5.3 |
| @uppy/core             |   3.9.2 | @uppy/utils            |   5.7.3 |
| @uppy/dashboard        |   3.7.4 | @uppy/xhr-upload       |   3.6.3 |
| @uppy/image-editor     |   2.4.3 | uppy                   |  3.22.2 |

- @uppy/core: fix plugin detection (Antoine du Hamel / #4951)
- @uppy/core,@uppy/utils: Introduce `ValidateableFile` & move `MinimalRequiredUppyFile` into utils (Antoine du Hamel / #4944)
- meta: uppy: fix bundle builder (Antoine du Hamel / #4950)
- @uppy/core: improve `UIPluginOptions` types (Merlijn Vos / #4946)
- @uppy/companion-client: fix body/url on upload-success (Merlijn Vos / #4922)
- @uppy/utils: remove EventManager circular reference (Merlijn Vos / #4949)
- @uppy/dashboard: MetaEditor + ImageEditor - new state machine logic (Evgenia Karunus / #4939)
- meta: disable `@typescript-eslint/no-non-null-assertion` lint rule (Antoine du Hamel / #4945)
- @uppy/companion-client: remove unnecessary `'use strict'` directives (Antoine du Hamel / #4943)
- @uppy/companion-client: type changes for provider-views (Antoine du Hamel / #4938)
- meta: bump ip from 1.1.8 to 1.1.9 (dependabot[bot] / #4941)
- @uppy/companion-client: update types (Antoine du Hamel / #4927)
Murderlon added a commit that referenced this pull request Feb 26, 2024
* main:
  @uppy/file-input: refactor to TypeScript (#4954)
  Release: uppy@3.22.2 (#4952)
  @uppy/core: fix plugin detection (#4951)
  Introduce `ValidateableFile` & move `MinimalRequiredUppyFile` into utils (#4944)
  uppy: fix bundle builder (#4950)
  @uppy/core: improve `UIPluginOptions` types (#4946)
  @uppy/companion-client: fix body/url on upload-success (#4922)
  @uppy/utils: remove EventManager circular reference (#4949)
  MetaEditor + ImageEditor - new state machine logic (#4939)
  meta: disable `@typescript-eslint/no-non-null-assertion` lint rule (#4945)
  remove unnecessary `'use strict'` directives (#4943)
  @uppy/companion-client: type changes for provider-views (#4938)
  meta: bump ip from 1.1.8 to 1.1.9 (#4941)
mifi added a commit that referenced this pull request Feb 28, 2024
* prevent "any" which disables typechecking

* make body optional in response (because it is)

* add comment
@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
3 participants