Skip to content

Fixes #293, added property url to BaseResponse. #321

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

Closed
wants to merge 1 commit into from
Closed

Fixes #293, added property url to BaseResponse. #321

wants to merge 1 commit into from

Conversation

yizhepku
Copy link

@yizhepku yizhepku commented Sep 15, 2019

The final url is extract from XMLHttpRequest.responseURL(on browsers) or HttpClientResponse.redirects.last.location(on vm).
Also added tests for vm.
No tests were written for browser, since server tests for brower is broken and needs to be fixed.

The final url is extract from XMLHttpRequest.responseURL(on browsers)
or HttpClientResponse.redirects.last.location(on vm).
Also added tests for vm.
No tests were written for browser, since server tests for brower is
broken and needs to be fixed.
natebosch added a commit that referenced this pull request May 25, 2022
Closes #321, closes #623, closes #692
Fixes #293

Add a Uri field to BaseResponse with the final, potentially redirected,
url for the content.

The field is nullable for backwards compatibility - TODO: consider if
this should be non-nullable from the start, or if it can be published
nullable first and become non-nullable (and the constructor arg
required) in the next breaking release.

This may break tests which use bocks but do not mock the field used to
read the URL.
@natebosch natebosch mentioned this pull request May 25, 2022
@natebosch
Copy link
Member

If we are able to roll out this feature I think we'll use a Uri field instead of String. I'll pick this up in #699

@natebosch natebosch closed this May 25, 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 this pull request may close these issues.

4 participants