Closed froatsnook closed 9 years ago
Regarding this one https://github.com/request/request/issues/1464
@froatsnook I think you are overthinking it.
baseUrl
should be String.baseUrl
is used, uri/url
should be string also.self.uri = url.parse(self.uri)
How that sounds? I understand your point of view to make everything bulletproof but is it logical in the first place? You are clearly stating what is the expected behavior in the README (the example), the rest of the code is just trying to prevent some potential bugs introduced by the user, that might happen :)
That sounds much easier. Should I squash everything to one commit?
Also, should baseUrl in README be lower since it's not as important as some of the first parameters?
Yes, some of the previous commits will become obsolete in this case :+1:
As for the README I think this place is fine, as it is related to the uri/url
parameter.
Good job @froatsnook I really like how tight and clean the implementation is looking. Also I really do like the new naming of the test cases, the previous ones were a bit odd :)
A couple of notes:
baseUrl
and the url
option?I just noticed that the tests are throwing some errors when executed via tape
and thus the coveralls reports are missing, but still you can execute
./node_modules/.bin/istanbul cover ./node_modules/tape/bin/tape tests/test-*.js
on your localhost and navigate to /path/to/request/coverage/lcov-report/request/request.js.html
to see the coverage report
Thanks @simov, done and done :). I think I thought test names had to be camelCase from glancing at test-body.js... well that's my story and I'm sticking to it! Thanks for the tip on code coverage. That's really useful.
Looking good to me, lets see if anyone else have something to add, otherwise I'll merge it.
Hey Im looking over this feature today, can you give me a little time to give feedback before merging?
Sure :+1:
The docs should have some examples of how this works.
I also don't think we should enforce that this should give me an error.
request('/home', {baseUrl: 'http://localhost:8080'}, function(err, res, body) {
})
I would expect this to make a request to http://localhost:8080/home
We also should probably just give an error if we see a path with //
. So:
request('//home', {baseUrl: 'http://localhost:8080'}, function(err, res, body) {
})
Would give an error. That's because //
is used in paths like: //cdn.com/jsfile.js
which would mean that its a whole new url with domain and blah.
Example would be good, also request('/path'
and request('path'
should return the same result I agree.
@seanstrom In the current version that case is covered since an error is thrown when the uri
starts with /
. The reasoning behind that is that it doesn't make sense to have a baseUrl
like http://example.com/api/
and a uri
of /absolute/path
.
@seanstrom @simov But maybe it's a lot less confusing if paths like that are just treated as relative paths and concatenated so that request('http://example.com/api/', { baseUrl: '/absolute/path' })
would request http://example.com/api/absolute/path
. If that's the consensus, I'd be happy to update the code and tests.
The example in your last comment is incorrect, although that's the idea exactly.
In other words both
request('/absolute/path', { baseUrl: 'http://example.com/api/' })
and
request('absolute/path', { baseUrl: 'http://example.com/api' })
should work
@simov OK I definitely agree that it's better like that. Will update.
@froatsnook in your first example you used: http://example.com/api/
+ /absolute/path
To the user I would expect at worst that we make a request with the url
of http://example.com/api//absolute/path
. But realistically we change that to be http://example.com/api/absolute/path
. I think that's how my browser handles it.
Also the tests themselves should test that it gets the correct error. Currently it just checks that it's not null
Okay im pretty with this, thanks for all the good work @froatsnook :+1: @simov do you have anymore feedback?
I'm fine with it too, lets see how it goes for a while, I might use this option in some of my projects as well.
@froatsnook you can specify which issues you are closing or fixing in your PR's description (see above).
Closes [link or #id]
Closes [link or #id]
Fixes [link or #id]
Fixes [link or #id]
That way the corresponding issues will be closed automatically once this PR got merged in. You can specify such things in the commit messages as well, but that can get a bit annoying sometimes :)
:+1:
@seanstrom Thanks for all the suggestions and the kind words.
@simov Thanks for the info.
Great work guys
This pull request contains bug when working with redirections. Details in my pull request #1481 .
Closes #1464 Closes #1476
Some remaining questions: