Feature/base url

[froatsnook]

Closes #1464
Closes #1476

Some remaining questions:

• What to do when baseUrl has a path component, like http://example.com/base/path? Append path or ignore?
• What to do when baseUrl has query parameters? Ignore or append/replace?
• More tests needed?

[simov]

Regarding this one #1464

@froatsnook I think you are overthinking it.

1. baseUrl should be String.
2. If baseUrl is used, uri/url should be string also.
3. If not throw error
4. Otherwise concatenate them, again throw runtime error if the behavior doesn't follow the one specified in the README
5. Put this check before self.uri = url.parse(self.uri)
6. Let request do the rest

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 :)

[froatsnook]

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?

[simov]

Yes, some of the previous commits will become obsolete in this case 👍

As for the README I think this place is fine, as it is related to the uri/url parameter.

[simov]

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:

1. The documentation is looking great, but can you add a really short explanation about the expected data type for the baseUrl and the url option?
2. Can you add some tests for the error cases?

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

[froatsnook]

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.

[simov]

Looking good to me, lets see if anyone else have something to add, otherwise I'll merge it.

[seanstrom]

Hey Im looking over this feature today, can you give me a little time to give feedback before merging?

[simov]

Sure 👍

[seanstrom]

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

[seanstrom]

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.

[simov]

Example would be good, also request('/path' and request('path' should return the same result I agree.

[froatsnook]

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

[froatsnook]

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

[simov]

The example in your last comment is incorrect, although that's the idea exactly.

[simov]

In other words both
request('/absolute/path', { baseUrl: 'http://example.com/api/' })
and
request('absolute/path', { baseUrl: 'http://example.com/api' })
should work

[froatsnook]

@simov OK I definitely agree that it's better like that. Will update.

[seanstrom]

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

[seanstrom]

Also the tests themselves should test that it gets the correct error. Currently it just checks that it's not null

[seanstrom]

we probably should say that "uri must be a string" instead of "options.uri", since we can just pass the uri value directly.

[seanstrom]

actually disregard this comment. Seems like we use this verbiage elsewhere

[seanstrom]

Okay im pretty with this, thanks for all the good work @froatsnook 👍
@simov do you have anymore feedback?

[simov]

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 :)

👍

[froatsnook]

@seanstrom Thanks for all the suggestions and the kind words.

@simov Thanks for the info.

[giladbeeri]

Great work guys

[burningtree]

This pull request contains bug when working with redirections. Details in my pull request #1481 .