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

Fix: Utilize user-defined onSuccess, onError, and onProgress callbacks in @uppy/tus #4674

Merged
merged 5 commits into from
Sep 18, 2023

Conversation

cgoinglove
Copy link
Contributor

This PR fixes the issue where user-defined onSuccess, onError, and onProgress callbacks were being ignored in the Tus plugin. While these options were accepted during initialization, they were not being called in the respective events.

This change ensures that if a user has provided any of these callbacks, they will be called in addition to the internal handlers.

Changes Made

  • Utilized user-defined onSuccess, onError, and onProgress callbacks in the Tus plugin.
  • Updated the code to check for and invoke user-defined callbacks if provided.

@arturi
Copy link
Contributor

arturi commented Sep 7, 2023

Seems like a good idea indeed. @Acconut, WDYT?

@arturi arturi requested review from arturi and Acconut September 7, 2023 15:12
@Acconut
Copy link
Member

Acconut commented Sep 7, 2023

It looks fine by me. But it's a question for you whether you want to allow users to receive these events using the tus-js-client callbacks and the Uppy events. That's two different approaches. Maybe you only want to support one approach.

@cgoinglove
Copy link
Contributor Author

cgoinglove commented Sep 7, 2023

@arturi @Acconut

Thank you for taking the time to review my PR, especially since this is my first PR.

Thank you for your feedback. I understand that Uppy already has a .on('upload-success') event that serves a similar purpose. However, the idea behind this PR is to offer flexibility to the end-users who are accustomed to the tus-js-client callbacks like onSuccess, onError, and onProgress.

While it's true that both onSuccess and .on('upload-success') occur at the same point, providing both options gives users the freedom to choose based on their specific needs or familiarity with one approach over the other. Some users might find it more convenient to stick with tus-js-client callbacks if they are already using them in other parts of their application. Others might prefer the Uppy's event-based approach. In either case, I believe that the responsibility should lie with the users to pick the approach that best suits their needs. 🤣

@Murderlon
Copy link
Member

But it's a question for you whether you want to allow users to receive these events using the tus-js-client callbacks and the Uppy events. That's two different approaches. Maybe you only want to support one approach.

Technically this was already possible in both. From the docs:

All options are passed to tus-js-client and we document the ones here that are required, added, or changed. This means you can also pass functions like onAfterResponse.

We recommended taking a look at the API reference from tus-js-client to know what is supported.

This PR just makes passing onError andonProgress not silently do nothing. So I would be okay with this change too.

@arturi arturi merged commit e400d4c into transloadit:main Sep 18, 2023
github-actions bot added a commit that referenced this pull request Sep 18, 2023
| Package            | Version | Package            | Version |
| ------------------ | ------- | ------------------ | ------- |
| @uppy/companion    |   4.9.0 | @uppy/locales      |   3.3.1 |
| @uppy/compressor   |   1.0.3 | @uppy/tus          |   3.3.0 |
| @uppy/dashboard    |   3.5.3 | uppy               |  3.16.0 |
| @uppy/image-editor |   2.2.0 |                    |         |

- @uppy/tus: Fix: Utilize user-defined onSuccess, onError, and onProgress callbacks in @uppy/tus (choi sung keun / #4674)
- @uppy/dashboard: Make file-editor:cancel event fire when the Image Editor “cancel” button is pressed (Artur Paikin / #4684)
- @uppy/companion: add missing credentialsURL for box (Mikael Finstad / #4681)
- @uppy/companion: remove s3 endpoints if s3 disabled (Mikael Finstad / #4675)
- meta: use latest Node.js version for tests (Antoine du Hamel / #4662)
- meta: Improve Contributing.md (Evgenia Karunus / #4633)
- @uppy/compressor: update file.meta.name after compression, becase format/extension might have changed (Artur Paikin / #4645)
- @uppy/companion: Onedrive refresh tokens (Mikael Finstad / #4655)
- @uppy/companion: catch "invalid initialization vector" instead of crashing (Mikael Finstad / #4661)
- @uppy/image-editor: Improve image rotation (Evgenia Karunus / #4639)
- @uppy/locales: Feature/updating i18n farsi (Parsa Arvaneh / #4638)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants