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

Tus plugin onBeforeRequest does not support Promises #3710

Closed
vp-aridhia opened this issue May 10, 2022 · 1 comment · Fixed by #3712
Closed

Tus plugin onBeforeRequest does not support Promises #3710

vp-aridhia opened this issue May 10, 2022 · 1 comment · Fixed by #3712
Labels

Comments

@vp-aridhia
Copy link

Hello, I found a small issue with the tus plugin. We're using uppy and tus for file uploads and came across a auth token issue raised internally. I found that onBeforeRequest will solve our issue, by adding the latest auth token in this handler and not using the headers options which aren't synced with ongoing uploads, on the condition it supports promises.

Our used versions are:

There's a slight mismatch in the @uppy/tus plugin, at the onBeforeRequest optional function.

https://github.com/tus/tus-js-client/blob/master/docs/api.md#onbeforerequest

You can also return a Promise if you need to perform some calculations before the request is sent:

onBeforeRequest: function (req) {
    return new Promise(resolve => {
        var xhr = req.getUnderlyingObect()
        xhr.withCredentials = true
        resolve()
    })
}

Tus supports promises, but @uppy/tus plugin wraps the optional function and calls it without handling the case when it's a promise.

https://github.com/transloadit/uppy/blob/main/packages/%40uppy/tus/src/index.js#L211

if (typeof opts.onBeforeRequest === 'function') {
  opts.onBeforeRequest(req)
}

I think it would be a good addition to also have promises enabled on this endpoint. Currently, I'm subclassing the plugin and replacing the bits I need.

Let me know if I missed something. I might be able to help with a PR

@aduh95
Copy link
Contributor

aduh95 commented May 10, 2022

Thanks for the report, I've opened #3712 to address this.

aduh95 added a commit that referenced this issue May 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants