@uppy/companion: use deferred length for tus streams #4697
Conversation
with tus as recommended in tus/tus-js-client#606 because sometimes HEAD reports an incorrect size
Murderlon
changed the title
|
@mifi I was literally looking into what it would take to implement this last week and I'm STOKED to see your PR (because I was quite lost 🥴)! Just to highlight the need for this, my colleague just checked the returned file size from S3 100 times... while they were mostly reported the same, there was some variation: Are you at all worried about the failing End-to-end tests? Seems like it's just a dependency issue 🧐 |
|
that's a nice insight, thanks. we can't always trust these darn computers 🤔 e2e tests seem to have become our neglected child unfortunately 😢 we need to make them more stable but haven't prioritized it yet |
|
oh and you're absolutely right. this time it was indeed a build error. i've run |
| Package | Version | Package | Version | | ------------------------- | ------- | ------------------------- | ------- | | @uppy/audio | 1.1.3 | @uppy/store-default | 3.0.4 | | @uppy/aws-s3 | 3.3.1 | @uppy/store-redux | 3.0.4 | | @uppy/aws-s3-multipart | 3.7.0 | @uppy/svelte | 3.1.0 | | @uppy/companion | 4.9.1 | @uppy/thumbnail-generator | 3.0.5 | | @uppy/companion-client | 3.4.1 | @uppy/transloadit | 3.3.1 | | @uppy/compressor | 1.0.4 | @uppy/tus | 3.3.1 | | @uppy/core | 3.5.1 | @uppy/utils | 5.5.1 | | @uppy/dashboard | 3.5.4 | @uppy/webcam | 3.3.3 | | @uppy/image-editor | 2.2.1 | @uppy/xhr-upload | 3.4.1 | | @uppy/remote-sources | 1.0.4 | uppy | 3.17.0 | - meta: add Prettier (Antoine du Hamel / #4707) - @uppy/aws-s3-multipart: retry signature request (Merlijn Vos / #4691) - meta: update linter config to cover more files (Mikael Finstad / #4706) - @uppy/image-editor: ImageEditor.jsx - remove 1px black lines (Evgenia Karunus / #4678) - meta: delete `.yarn/releases/yarn-3.4.1.cjs` (Antoine du Hamel) - meta: fix linter errors (Antoine du Hamel / #4704) - @uppy/utils: test: migrate to Vitest for Uppy core and Uppy plugins (Antoine du Hamel / #4700) - meta: run corepack yarn (Mikael Finstad) - @uppy/companion: upgrade TS target (Mikael Finstad / #4670) - @uppy/companion: use deferred length for tus streams (Mikael Finstad / #4697) - @uppy/companion-client: fix a refresh token race condition (Mikael Finstad / #4695) - meta: add companion hotfix doc (Mikael Finstad / #4683) - meta: run type checks also for companion and add files to docker (Mikael Finstad / #4688) - @uppy/svelte: revert breaking change (Antoine du Hamel / #4694) - meta: Update yarn.lock (Artur Paikin) - @uppy/companion: fix instagram/facebook auth error regression (Mikael Finstad / #4692) - @uppy/aws-s3-multipart: aws-s3-multipart - call `#setCompanionHeaders` in `setOptions` (jur-ng / #4687) - @uppy/svelte: Upgrade Svelte to 4 (frederikhors / #4652) - @uppy/companion: add test endpoint for dynamic oauth creds (Mikael Finstad / #4667) - meta: fix VITE_COMPANION_ALLOWED_HOSTS (Mikael Finstad / #4690) - @uppy/companion: fix edge case for pagination on root (Mikael Finstad / #4689) - @uppy/companion: fix onedrive pagination (Mikael Finstad / #4686)
Looking back at this, it can be suboptimal for the tus server to not know the size at all. If HEAD doesn't deliver the correct amount, it would also be possible to use the Content-Length header in the response to the GET request. If present, it should be much more accurate. |
|
In which cases does Content-Length from HEAD differ from Content-Length from GET? IIRC the original problem here was that sometimes the Content-Length returned from HEAD was plain wrong, causing tus client to break, so to be safe, we just ignore content length completely because it cannot always be trusted. I’d imagine if HEAD responds with an incorrect length, then GET would do the same (for the same URL) |
|
A HEAD response should return the same Content-Length header as the corresponding GET requests, yes. I can imagine that the header values inside the same request have a higher likelihood to be consistent and therefore GET might deliver the more promising Content-Length value. In addition, it could mean that Companion doesn't have to send the HEAD request at all. |
with tus
as recommended in tus/tus-js-client#606
because sometimes HEAD reports an incorrect size