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

Adding back missing Request and Response fields #78

Merged
merged 2 commits into from May 23, 2017

Conversation

donny-dont
Copy link

There were a few fields that were not present in previous pull requests.

Also wondering if persistentConnection, followRedirects, and maxRedirects should be modifiable within the change method of Request.

@nex3
Copy link
Member

nex3 commented May 16, 2017

Following shelf's lead, I'd like to make these part of the context map rather than including them on the main objects because they're only meaningful for server clients, and can't be supported on the browser.

@donny-dont
Copy link
Author

@nex3 works for me! If that's the case then next up is the dart:io client. I have some stuff for that locally.

So with that is there a preference for the name within context for those values?

@nex3
Copy link
Member

nex3 commented May 17, 2017

I think io.persistentConnection?

@donny-dont
Copy link
Author

donny-dont commented May 22, 2017

@nex3 do we care about reasonPhrase anymore? Should it end up in the context of the returned Response?

@nex3
Copy link
Member

nex3 commented May 22, 2017

I think reasonPhrase is still useful as a field—it should be available from all implementations, too.

@donny-dont
Copy link
Author

K works for me.

I'm also wondering if url should get moved into Message. Both dart:html and dart:io apps can get the redirected url. I don't see any other way to get whether a request was redirected.

@nex3
Copy link
Member

nex3 commented May 22, 2017

It makes sense to expose that information, but I don't know that it makes sense to use the same name since the meaning is different. Maybe call it finalUrl on the response?

@donny-dont
Copy link
Author

Maybe location? https://en.wikipedia.org/wiki/HTTP_location

@nex3
Copy link
Member

nex3 commented May 22, 2017

Yeah, I like that.

@donny-dont donny-dont force-pushed the fix/request-response-fields branch from 73cfddb to 92b37ee Compare May 23, 2017 00:08
@donny-dont
Copy link
Author

@nex3 good to go.

@nex3
Copy link
Member

nex3 commented May 23, 2017

Shouldn't location come from the headers?

@donny-dont
Copy link
Author

Its only in the headers when there is a redirect or a newly created resource. I verified it through https://httpbin.org/ 's methods and only saw it if redirects were off.

Change the name?

@nex3
Copy link
Member

nex3 commented May 23, 2017

Maybe we should just have it be null if it's not set for those reasons, then.

@donny-dont
Copy link
Author

Okay so I don't think location is the best name anymore because it only seems to apply to actual redirects. Here's some example information using httpbin which has a redirect to URL endpoint.

I have some local patches to enable the dart:io client. It uses RedirectInfo to determine where the final request is and puts it at location when creating the Response.

Request when io.followRedirects is false.

REQUEST
URL: https://httpbin.org/redirect-to?url=http%3A%2F%2Fexample.com%2F

RESPONSE
URL: https://httpbin.org/redirect-to?url=http%3A%2F%2Fexample.com%2F
Status: 302
Headers: 
x-powered-by: Flask
access-control-allow-credentials: true
connection: close
location: http://example.com/
date: Tue, 23 May 2017 18:20:09 GMT
access-control-allow-origin: *
content-length: 0
via: 1.1 vegur
content-type: text/html; charset=utf-8
x-processed-time: 0.000843048095703
server: meinheld/0.6.1

So location is there and its pointing to the redirect.

Now if io.followRedirects is true the result is the following.

REQUEST
URL: https://httpbin.org/redirect-to?url=http%3A%2F%2Fexample.com%2F

RESPONSE
URL: http://example.com/
Status: 200
Headers: 

connection: close
cache-control: max-age=604800
last-modified: Fri, 09 Aug 2013 23:54:35 GMT
date: Tue, 23 May 2017 18:29:26 GMT
content-encoding: gzip
vary: Accept-Encoding
content-type: text/html
server: ECS (cpm/F9D5)
accept-ranges: bytes
content-length: 606
etag: "359670651+gzip"
x-cache: HIT
expires: Tue, 30 May 2017 18:29:26 GMT

With the redirect followed location is not present.

With this information I'm leaning towards moving url to the base Message class. They both have a url associated with them. If the url is not added to the Response then there is no way to determine if there was a redirect.

@nex3
Copy link
Member

nex3 commented May 23, 2017

I don't like the idea of using the name url, since the semantics are different. I think I still like finalUrl.

@donny-dont
Copy link
Author

And named changed as requested. Can't think of a better name for it 🤷‍♂️

@nex3
Copy link
Member

nex3 commented May 23, 2017

Is this ready to land now?

@donny-dont
Copy link
Author

Yep :shipit:

@nex3 nex3 merged commit 128c0bc into dart-lang:master May 23, 2017
@donny-dont donny-dont deleted the fix/request-response-fields branch May 23, 2017 21:50
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.

None yet

3 participants