Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Adding responseURL to jqXHR#4405

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
csmadhav wants to merge3 commits intojquery:main
base:main
Choose a base branch
Loading
fromcsmadhav:4339-adding-responseurl-field-jqxhr

Conversation

csmadhav
Copy link

Summary

Fixes:#4339
I've taken some references from#1615 (which needed to be resurrected)
original PR was from@xKerman
Since recently support for some browsers was dropped in jquery respective changed were also added.
Please ignore the previous PR (closed already)

Checklist

xKerman reacted with thumbs up emoji
@csmadhavcsmadhav changed the titleAjax: Adding responseURL to jqXHRAdding responseURL to jqXHRMay 19, 2019
Copy link
Member

@mgolmgol left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I added some comments.

Copy link
Member

@mgolmgol left a comment

Choose a reason for hiding this comment

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

Sorry for the delay. See my latest comments.

@csmadhavcsmadhavforce-pushed the4339-adding-responseurl-field-jqxhr branch from467cea1 to25cee2dCompareDecember 18, 2019 14:09
@csmadhav
Copy link
Author

sorry for updating this late.

@mgolmgol added the Ajax labelJan 8, 2020
xhr.statusText,

// For XHR2 non-text, let the caller handle it (gh-2498)
xhr.statusText, // For XHR2 non-text, let the caller handle it (gh-2498)
Copy link
Member

Choose a reason for hiding this comment

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

This comment shouldn't have been moved, please move it back below.

errorURL = "http://example.invalid",
redirectAndSuccessURL = url( "mock.php?action=responseURL&url=" + encodeURIComponent( successURL ) ),
redirectAndErrorURL = url( "mock.php?action=responseURL&url=" + encodeURIComponent( errorURL ) ),
jsonpURL = baseURL + "mock.php?action=jsonp&callback=?",
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason why this usesbaseURL directly instead of theurl function?

@@ -711,7 +711,7 @@ jQuery.extend( {
}

// Callback for when everything is done
function done( status, nativeStatusText, responses, headers ) {
function done( status, nativeStatusText, responses, headers, responseURL ) {
Copy link
Member

Choose a reason for hiding this comment

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

Unfortunately, this function signature is public API as that's the shape ofcompleteCallback as described here:https://api.jquery.com/jQuery.ajaxTransport/

This is a new parameter at the end so it's not a breaking change. However, we need to think if this is a good API as 5 unnamed parameters is quite a lot.

(from my comment from#4405 (comment))

Copy link
Member

@mgolmgolMar 1, 2020•
edited
Loading

Choose a reason for hiding this comment

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

FYI: I opened#4634 to make passingresponseURL possible without adding more positional parameters to this function.

Copy link
Member

@mgolmgol left a comment

Choose a reason for hiding this comment

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

See newest comments.

@mgol
Copy link
Member

Adding the "Needs review" label back to discuss#4405 (comment)

mgol added a commit to mgol/jquery that referenced this pull requestMar 1, 2020
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestDec 8, 2020
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
Base automatically changed frommaster tomainFebruary 1, 2021 22:02
@mgol
Copy link
Member

Closing & re-opening the PR to trigger the EasyCLA check...

@mgolmgol closed thisSep 17, 2021
@mgolmgol reopened thisSep 17, 2021
@linux-foundation-easycla

CLA Not Signed

mgol added a commit to mgol/jquery that referenced this pull requestJul 10, 2023
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestSep 21, 2023
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestNov 20, 2023
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestSep 10, 2024
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestNov 25, 2024
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull requestMay 4, 2025
Apart from the existing API:```jsfunction( status, statusText, responses, headers ) {}```a new API is now available:```jsfunction( { status, statusText, responses, headers } ) {}```This makes it possible to add new parameters in the future without relying ontheir order among parameters and being able to provide them selectively.Refjquerygh-4405
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@mgolmgolmgol requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Adding responseURL field to jqXHR
3 participants
@csmadhav@mgol@timmywil
[8]ページ先頭
©2009-2025 Movatter.jp