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

Wait until beforeSend header is returned before sending xhr request #467

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erasromani
Copy link

In some cases, beforeSend method passes is asynchronous or returns a promise. For example, when the xhr request requires addition of an access token in the header and the retreival of the access token is done asynchronously, the beforeSend methos is therefore asynchronous. In this case, the current implementation does not wait for the header to be returned before sending the xhr request.

@netlify
Copy link

netlify bot commented Aug 4, 2022

Deploy Preview for cornerstone-wado-image-loader failed.

Name Link
🔨 Latest commit 5f8cb2c
🔍 Latest deploy log https://app.netlify.com/sites/cornerstone-wado-image-loader/deploys/62eb80c7772c030008b1654d

@timtong1982
Copy link

with the code of your PR commit, the beforeSend is still sync

@sedghi
Copy link
Member

sedghi commented Dec 1, 2022

@wayfarer3130 Can you please look at this?

@sedghi sedghi requested a review from wayfarer3130 December 1, 2022 02:30

let beforeSendHeaders = {};
try{
beforeSendHeaders = options.beforeSend(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is still a synchronous call. This is also not the current version - there is an options.open method that runs before on line 23. Since this is already a promise, it should be easy to change this into an async function on line 4, then return the result directly down below.

Copy link
Member

Choose a reason for hiding this comment

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

Yes it seems to be outdated base, @erasromani can you please rebase?

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.

4 participants