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: add onBeforeRequest option #2611

Merged
merged 3 commits into from Nov 16, 2020
Merged

Conversation

bedgerotto
Copy link
Contributor

The onBeforeRequest callback cant be changed by Tus options after the changes on the issue #2518

In this case, I think that we could skip the default callback definition if the callbacck was passed through Tus options

@arturi
Copy link
Contributor

arturi commented Nov 4, 2020

@Acconut what do you think?

@Acconut
Copy link
Member

Acconut commented Nov 5, 2020

It would be better if the withCredentials option keeps working, even if onBeforeRequest is supplied. This is possible, so I don't see a reason for this behavior as it could be very confusing for users. How about this code:

        uploadOptions.onBeforeRequest = (req) => {
          const xhr = req.getUnderlyingObject()
          xhr.withCredentials = !!opts.withCredentials

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

@bedgerotto
Copy link
Contributor Author

In this case, I was overwriting onBeforeRequest to manually set xhr.withCredentials as true because, even I set withCredentials option on Tus configuration as true I still unable to send and receive cookies through subdomains.

Maybe, it's related with this another issue?(#2443 )

@Acconut
Copy link
Member

Acconut commented Nov 5, 2020

I don't understand your last comment, to be honest. The code I posted above is not meant as a suggestion for how you can fix the issue in your application. It's a suggestion for how this PR should be implemented instead.

@bedgerotto
Copy link
Contributor Author

Sorry, I got it wrong.

I'll try your suggestion and update this PR. Thanks

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! Let me know, if you made the changes, so I can review them, @bedgerotto!

@bedgerotto
Copy link
Contributor Author

@Acconut I checked out your suggestion and it didn't work because it is overwriting the uploadOptions.onBeforeRequest and then trying to call the same method if it is not a function.

But, based on this approach I have tested the following code and it works well once it is attached on the received opts

uploadOptions.onBeforeRequest = (req) => {
    const xhr = req.getUnderlyingObject()
    xhr.withCredentials = !!opts.withCredentials

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

Copy link
Member

@Acconut Acconut left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A test would be nice but that's up to the Uppy team to decide. From a tus-js-client POV, this looks good 👍

@goto-bus-stop goto-bus-stop changed the title Allow onBeforeRequest from host application tus: add onBeforeRequest option Nov 16, 2020
@goto-bus-stop goto-bus-stop merged commit 256c06e into transloadit:master Nov 16, 2020
@goto-bus-stop
Copy link
Contributor

thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants