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

Support URLs that have a path component #843

Closed
wants to merge 4 commits into from
Closed

Conversation

micahr
Copy link
Member

@micahr micahr commented Jun 20, 2015

Combine both PRs #633 and #628

Pull the logic from #633 and clean it up a bit
Pull the tests from #628

Thanks to both @nakosung and @calebbrown for their contributions.

The goal of this PR is to use the path from both the Client options and the requested url.

For example, when a user creates a client for a non-root url, any subsequent requests should use that as part of the request path.

var client = restify.createClient({
    url: 'http://127.0.0.1/str'
});

client.get('/foo', function (err, req, res, data){});

The resulting request should go to http://127.0.0.1/str/foo.

nakosung and others added 4 commits June 19, 2015 23:44
Fixed erroneous path generation like below:
url[http://localhost:3000/something] + get[/foo] 
  ->  (should be)     http://localhost:3000/something/foo
  -x->(should not be) http://localhost:3000/foo
This was referenced Jun 20, 2015
if (opts.path && this.url.path &&
opts.path !== '/' && this.url.path !== '/') {
// clean up the url path, stripping the last /
opts.path = utils.sanitizePath([this.url.path, opts.path].join('/'));
Copy link
Member

Choose a reason for hiding this comment

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

Based on #692, I think it's safe to say that we probably don't want to strip trailing slashes. Make the users be explicit?

Copy link
Member

Choose a reason for hiding this comment

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

Also, could probably use url.resolve() in favor of [].join('/');

Copy link
Member Author

Choose a reason for hiding this comment

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

This wouldn't affect #692 as this is a http client making requests to any other http server / endpoint, not the server managing routes.

url.resolve() won't work if the request url has a / at the beginning, it will replace whatever path is already at the beginning of the path.

url.resolve('/foo/bar', '/baz') === '/baz';

Copy link
Member

Choose a reason for hiding this comment

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

That's a bummer, maybe we should look into something like URIjs, there's a lot of URL mugging that should be centralized (plugins too).

Re: above sorry, I should have been clearer, I just think that we should be consistent, presumably if users need strict URLs on the server, them they need a client that can hit those strict URLs. If we strip trailing slashes automatically them it's impossible to do so. Maybe we can do a strict:true option?

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, we should definitely standardize the management of urls via the server and client libraries.

@yunong
Copy link
Member

yunong commented Jun 22, 2015

This is definitely a feature we want. But I'm going to lean on not merging this specific PR. We should figure out the URL interface and standardize on that per what @micahr suggested. Will close and file #846 to that effect.

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.

None yet

4 participants