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

Feature/base url #1469

Merged
merged 6 commits into from
Mar 13, 2015
Merged

Feature/base url #1469

merged 6 commits into from
Mar 13, 2015

Conversation

froatsnook
Copy link
Contributor

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
Copy link
Member

simov commented Mar 10, 2015

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
Copy link
Contributor Author

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
Copy link
Member

simov commented Mar 10, 2015

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
Copy link
Member

simov commented Mar 11, 2015

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

coverage

@froatsnook
Copy link
Contributor Author

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
Copy link
Member

simov commented Mar 11, 2015

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

@seanstrom
Copy link
Contributor

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

@simov
Copy link
Member

simov commented Mar 12, 2015

Sure 👍

@seanstrom
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Member

simov commented Mar 12, 2015

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

@froatsnook
Copy link
Contributor Author

@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
Copy link
Contributor Author

@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
Copy link
Member

simov commented Mar 12, 2015

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

@simov
Copy link
Member

simov commented Mar 12, 2015

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

@froatsnook
Copy link
Contributor Author

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

@seanstrom
Copy link
Contributor

@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
Copy link
Contributor

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

}

if (typeof self.uri !== 'string') {
return self.emit('error', new Error('options.uri must be a string when using options.baseUrl'))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@seanstrom
Copy link
Contributor

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

@simov
Copy link
Member

simov commented Mar 13, 2015

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
Copy link
Contributor Author

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

@simov Thanks for the info.

simov added a commit that referenced this pull request Mar 13, 2015
@simov simov merged commit 9f844e2 into request:master Mar 13, 2015
@giladbeeri
Copy link

Great work guys

@burningtree
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

relative URLs with the request library Allow configuring a default base url
5 participants