Skip to content

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Adding responseURL to jqXHR #4405

wants to merge 3 commits into from

Conversation

@csmadhav
Copy link

@csmadhav csmadhav commented May 19, 2019

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

@csmadhav csmadhav changed the title Ajax: Adding responseURL to jqXHR Adding responseURL to jqXHR May 19, 2019
mgol
mgol reviewed May 19, 2019
View reviewed changes
Copy link
Member

@mgol mgol 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.

@tchapi tchapi mentioned this pull request Jun 27, 2019
Closed
mgol
mgol requested changes Jul 18, 2019
View reviewed changes
Copy link
Member

@mgol mgol 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.

@csmadhav csmadhav force-pushed the 4339-adding-responseurl-field-jqxhr branch from 467cea1 to 25cee2d Compare December 18, 2019 14:09
@csmadhav
Copy link
Author

csmadhav commented Dec 22, 2019

sorry for updating this late.

@mgol mgol added the Ajax label Jan 8, 2020
mgol
mgol reviewed Jan 9, 2020
View reviewed changes
src/ajax/xhr.js
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

@mgol mgol Jan 9, 2020

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.

test/unit/ajax.js
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

@mgol mgol Jan 9, 2020

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 uses baseURL directly instead of the url function?

src/ajax.js

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

@mgol mgol Jan 9, 2020

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 of completeCallback 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

@mgol mgol Mar 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 passing responseURL possible without adding more positional parameters to this function.

mgol
mgol requested changes Jan 9, 2020
View reviewed changes
Copy link
Member

@mgol mgol 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

mgol commented Feb 24, 2020

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

@smarek smarek mentioned this pull request Mar 1, 2020
Merged
mgol added a commit to mgol/jquery that referenced this pull request Mar 1, 2020
@mgol
Ajax: Support an alternative completeCallback API for transports
62e7f2c 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
@mgol mgol mentioned this pull request Mar 1, 2020
Draft
4 tasks
@mgol mgol removed the Needs review label Mar 9, 2020
@AnnikaCodes AnnikaCodes mentioned this pull request Jul 27, 2020
Open
mgol added a commit to mgol/jquery that referenced this pull request Dec 8, 2020
@mgol
Ajax: Support an alternative completeCallback API for transports
dc6880a 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
Base automatically changed from master to main February 1, 2021 22:02
@mgol
Copy link
Member

mgol commented Sep 17, 2021

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

@mgol mgol closed this Sep 17, 2021
@mgol mgol reopened this Sep 17, 2021
@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Sep 17, 2021

CLA Not Signed

mgol added a commit to mgol/jquery that referenced this pull request Jul 10, 2023
@mgol
Ajax: Support an alternative completeCallback API for transports
d3bc8fa 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull request Sep 21, 2023
@mgol
Ajax: Support an alternative completeCallback API for transports
4885508 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull request Nov 20, 2023
@mgol
Ajax: Support an alternative completeCallback API for transports
0e22c4a 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull request Sep 10, 2024
@mgol
Ajax: Support an alternative completeCallback API for transports
bc5f82b 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull request Nov 25, 2024
@mgol
Ajax: Support an alternative completeCallback API for transports
0facb1a 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
mgol added a commit to mgol/jquery that referenced this pull request May 4, 2025
@mgol
Ajax: Support an alternative completeCallback API for transports
c27a3b8 
Apart from the existing API:

```js
function( status, statusText, responses, headers ) {}
```
a new API is now available:
```js
function( { status, statusText, responses, headers } ) {}
```

This makes it possible to add new parameters in the future without relying on
their order among parameters and being able to provide them selectively.

Ref jquerygh-4405
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mgol mgol mgol requested changes

Requested changes must be addressed to merge this pull request.

Assignees

No one assigned

Labels

Ajax

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Adding responseURL field to jqXHR

3 participants

@csmadhav @mgol @timmywil