-
Notifications
You must be signed in to change notification settings - Fork 2.1k
@uppy/utils,@uppy/xhr-upload: create and use new priority queue #6105
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
base: main
Are you sure you want to change the base?
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR introduces a new TaskQueue implementation to replace the existing RateLimitedQueue, providing better separation of concerns through a simpler, promise-based priority queue. The change is initially implemented in the xhr-upload plugin while maintaining backward compatibility with companion-client.
Key changes:
- New
TaskQueueclass with binary max-heap priority queue implementation - Updated xhr-upload to use TaskQueue instead of RateLimitedQueue
- Maintained legacy compatibility methods (
wrapPromiseFunction,abortOn) for companion-client integration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/@uppy/utils/src/TaskQueue.ts | New priority queue implementation with max-heap, abort support, and pause/resume functionality |
| packages/@uppy/utils/src/TaskQueue.test.ts | Basic test coverage for priority ordering, abort handling, and clear functionality |
| packages/@uppy/utils/src/index.ts | Exports for new TaskQueue types and class |
| packages/@uppy/xhr-upload/src/index.ts | Migrated from RateLimitedQueue to TaskQueue, simplified abort signal handling |
| packages/@uppy/locales/src/en_US.ts | Removed unused chooseFiles string and added failedToAddFiles |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Closes #4173, kind off
Problem
Current situation with
RateLimitedQueueCase in point: it's some sort of made up, monstrosity data structure trying to be too many things. It also has rate limiting and exponential backoff but that's already in
fetchertoo.Would be better if we had separation of concerns and proven data structures.
Solution
A "dumb" promise-based priority queue that doesn't care at all about what a promise does or if it needs to be retried. The promise inside determines if it has retrying and exponential backoff, such as a
fetcherpromise.To not make this a breaking change and a huge diff, we still implement this per uploader, starting with xhr-upload, and keep backwards compatibility in the interface so we can still pass it to
companion-client, which needs to share the same queue.Ideal future
When you think about it, it's odd that we implement a queue per uploader if a queue is so central to uppy working correctly. It's even more odd that we also have to inject that queue into
companion-clientper uploader for queue sharing.Ideally,
coreis responsible for having the queue. All uploaders would do is push a promise tocoreandcoredoesn't care if that promise is a tus, xhr, or S3 upload.