Support path in the base url #23

kaeedo · 2017-11-21T12:02:22Z

This PR adds the option to include an option to preserve the pathname specified in the base URL for the Rest Client. Default is false (maintains old behavior)

msftclas · 2017-11-21T12:02:30Z

All CLA requirements met.

bryanmacfarlane · 2017-11-21T13:12:15Z

package.json

  "description": "Node Rest and Http Clients for use with TypeScript",
  "main": "./RestClient.js",
  "scripts": {
    "build": "node make.js build",
    "test": "node make.js test",
    "samples": "node make.js samples"
  },
+  "engines": {
+    "node": ">=8.9.0",


node 8.9 / npm 5.1 is only a developer build time dependency. At runtime this should and needs to work on 6.x LTS and even back to node 5.10. I would remove this from the PR and make another issue to define runtime req (I want our CI to multi plex and validate on ideally >= node 4.x LTS but at least node >=6.x LTS)

bryanmacfarlane · 2017-11-21T13:14:18Z

lib/Util.ts


+    if (preservePath) {
+        combined.pathname = base.pathname + combined.pathname;


path.join? I think there is also interesting test cases with base not / having trailing slash and pathname not/having leading slash.

Using path.join now. Also fixes the trailing and leading slash problems

bryanmacfarlane · 2017-11-21T13:19:57Z

lib/Util.ts

 * @return {string} - resultant url 
 */
-export function getUrl(requestUrl: string, baseUrl?: string): string  {
+export function getUrl(requestUrl: string, baseUrl?: string, preservePath?: boolean): string  {


I'm struggling with whether this should be the default. In the normal case of just providing the hostname portion (no path) it does the right thing. In the case where the base url has a relative path, it does the right thing. I need to think about it.

This is pre-release about to go to 1.0 and this change should bump the minor version (so ^ with patch level won't drift into this behavior) and we can doc.

The other thought was flip to opt in to chop the relative path but if that's your intent, just provide the hostname portion. No need for another optional arg ...hmmmm

Thoughts?

Since this is still prerelease, my preference would be to never strip the relative path. Reason being similar to your thought about flipping to opt in. If the use case comes up that the base URL also has a path, then it would make sense to explicitly state it in the baseUrl parameter, and always expect it to be there. At that point the optional parameter can be completely removed. That would make this a breaking change though...

@kaeedo Could you add some examples of usage for this feature? Then we can speak to the explicit use cases. It often helps think things through and make the "right" choice more apparent. (based on the perspective of a consumer of the library). As well as what the consumer would have to do if we didn't have this feature.

It's tricky to balance where we draw the line of what's included and what isn't.

@kaeedo - I agree on your points. Bump the minor version with this change. That mitigates the pre-release breaking change since folks default to sliding on patch versions. And it's only a change when you're doing something non mainstream (passing base url with path but then expecting it not to be honored). Let's move forward without the arg. Add the test cases on trailing and leading slashes.

@stephenmichaelf
For the use case in my office, we have a reverse proxy configured to point to different docker containers:

https://www.contoso.com/firstcontainer https://www.contoso.com/secondcontainer https://www.contoso.com/thirdcontainer

I would like to create an instance of a RestClient such that https://www.contoso.com/firstcontainer is the baseUrl instead of just https://www.contoso.com and then having to prepend firstcontainer to every rest request

bryanmacfarlane · 2017-11-21T13:20:52Z

test/resttests.ts

@@ -84,7 +85,93 @@ describe('Rest Tests', function () {
            assert(err['statusCode'] == 500, "statusCode should be 500");
            assert(err.message && err.message.length > 0, "should have error message");
        }
-    });    
+    });
+


Add divider

//--------------------------------------------------------
// Path tests
//--------------------------------------------------------

kaeedo · 2017-11-22T07:52:09Z

Preserving path is now the default, and cannot be turned off. the version in package.json has been bumped to 0.15.0. Unit test for trailing slashes and no slashes are also there

bryanmacfarlane · 2017-12-01T00:58:27Z

please resolve conflicts (bump version to 0.16) and I'll review tomorrow.

bryanmacfarlane · 2017-12-01T13:12:37Z

lib/Util.ts


+    combined.pathname = path.join(base.pathname, combined.pathname);


Instead of path.join, let's try path.resolve.
So
'http://httpbin.org' and 'foo/bar' = 'http://httpbin.org/foo/bar'
'http://httpbin.org' and '/foo/bar' = 'http://httpbin.org/foo/bar'
'http://httpbin.org/baz' and 'foo/bar' = 'http://httpbin.org/baz/foo/bar'
'http://httpbin.org/baz' and '/foo/bar' = 'http://httpbin.org/foo/bar'

I sketched it out in a sketch PR.

#28

See my tests. You can merge in with your PR.

Also note I bumped to 1.0 and will release with tag of preview. After thinking about it, I think that's the best thing to do.

Thoughts?

The problem with using path.resolve on a Windows system, is that an absolute path will include a drive letter. For the test case

let res: string = util.getUrl('get/foo', 'http://httpbin.org/bar');
res comes out to http://httpbin.org/C:\bar\get\foo

bryanmacfarlane · 2017-12-01T15:19:44Z

not a problem - if you do something illogical like putting a drive letter in an url path it won't work. We're optimizing for the positive case scenarios (rooted and unrooted relative paths) Sent from Outlook<http://aka.ms/weboutlook>

________________________________ From: Kai <notifications@github.com> Sent: Friday, December 1, 2017 10:14:33 AM To: Microsoft/typed-rest-client Cc: Bryan MacFarlane; Comment Subject: Re: [Microsoft/typed-rest-client] Support path in the base url (#23) @kaeedo commented on this pull request.

________________________________ In lib/Util.ts<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Ftyped-rest-client%2Fpull%2F23%23discussion_r154368542&data=02%7C01%7Cbryanmac%40microsoft.com%7C568f6fb651ef4b28f08408d538ce3a4f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636477380748909514&sdata=lG6dyFfuGvbKdo9pF%2Fmgys9ElS4sksSfIC4MMm1El0g%3D&reserved=0>:

+ combined.pathname = path.join(base.pathname, combined.pathname); The problem with using path.resolve on a Windows system, is that an absolute path will include a drive letter. For the test case let res: string = util.getUrl('get/foo', 'http://httpbin.org/bar'); res comes out to http://httpbin.org/C:\bar\get\foo — You are receiving this because you commented. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FMicrosoft%2Ftyped-rest-client%2Fpull%2F23%23discussion_r154368542&data=02%7C01%7Cbryanmac%40microsoft.com%7C568f6fb651ef4b28f08408d538ce3a4f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636477380748909514&sdata=lG6dyFfuGvbKdo9pF%2Fmgys9ElS4sksSfIC4MMm1El0g%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAA4IDANxuhddyh4Bu-ABISySkyNDqQJGks5s8BfYgaJpZM4Qluss&data=02%7C01%7Cbryanmac%40microsoft.com%7C568f6fb651ef4b28f08408d538ce3a4f%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636477380748909514&sdata=wtOsaHdCUrKVXmuJLjxpshvREn6Mopy9IosKrTpwr1s%3D&reserved=0>.

bryanmacfarlane · 2017-12-01T15:24:14Z

test/resttests.ts

@@ -176,5 +177,27 @@ describe('Rest Tests', function () {
        assert(restRes.result.args.baz === 'top');
    });

+    //
+    // getUrl path tests


you're missing this test.

'/get/foo', 'http://httpbin.org/bar'

should result in 'http://httpbin.org/get/foo'

bryanmacfarlane · 2017-12-01T15:25:01Z

lib/Util.ts


-    combined.pathname = path.join(base.pathname, combined.pathname);
+    let basePathComponent: string = base.pathname === '/' ? '': base.pathname;
+    resultantUrl.pathname = path.posix.join(basePathComponent, resultantUrl.pathname);


use resolve. drive letter on windows not a problem. If you do something illogical like using a drive letter on windows, you will get a failure which is expected. We're optimizing for the positive rooted and unrooted cases.

I see what it's doing now.

bryanmacfarlane · 2017-12-02T02:24:08Z

Thanks!

bryanmacfarlane · 2017-12-02T02:30:51Z

published as typed-rest-client@preview

Centro1993 · 2020-08-10T16:44:05Z

Why is this not in Master? Just discovered that BaseUrl cannot be a relative path. Worked fine for development, but now that I want to take my project online, I want to redirect all my API Calls to /api.

Add optional parameter to preserve path from base url

55d8ca1

kaeedo mentioned this pull request Nov 21, 2017

Support paths in base url #4

Closed

bryanmacfarlane reviewed Nov 21, 2017

View reviewed changes

Kai Ito added 3 commits November 21, 2017 14:45

Use path.join instead of concatenation

b53c4bb

Remove forgotten console log

18fdc88

Remove parameter to toggle preservePath

053191c

bryanmacfarlane changed the title ~~Add optional parameter to preserve path from base url~~ Support path in the base url Dec 1, 2017

bryanmacfarlane added the enhancement label Dec 1, 2017

relpaths

42f98e9

bryanmacfarlane reviewed Dec 1, 2017

View reviewed changes

Kai and others added 3 commits December 1, 2017 14:38

Merge branch 'master' into master

bfa8753

Merge relpaths branch

0957f1c

Use path.posix.join to force forward slash

f41b1b5

bryanmacfarlane reviewed Dec 1, 2017

View reviewed changes

Fix unit tests

149eb43

bryanmacfarlane merged commit fc0e561 into microsoft:master Dec 2, 2017

bryanmacfarlane mentioned this pull request Dec 2, 2017

base url relative path support sketch (DO NOT MERGE) #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support path in the base url #23

Support path in the base url #23

kaeedo commented Nov 21, 2017

msftclas commented Nov 21, 2017 •

edited

Loading

bryanmacfarlane Nov 21, 2017

bryanmacfarlane Nov 21, 2017

kaeedo Nov 21, 2017

bryanmacfarlane Nov 21, 2017

kaeedo Nov 21, 2017

stephenmichaelf Nov 21, 2017

bryanmacfarlane Nov 21, 2017

kaeedo Nov 22, 2017

bryanmacfarlane Nov 21, 2017

kaeedo Nov 21, 2017

kaeedo commented Nov 22, 2017

bryanmacfarlane commented Dec 1, 2017

bryanmacfarlane Dec 1, 2017

kaeedo Dec 1, 2017

bryanmacfarlane commented Dec 1, 2017 via email

bryanmacfarlane Dec 1, 2017

bryanmacfarlane Dec 1, 2017

kaeedo Dec 1, 2017

bryanmacfarlane commented Dec 2, 2017

bryanmacfarlane commented Dec 2, 2017

Centro1993 commented Aug 10, 2020


		if (preservePath) {
		combined.pathname = base.pathname + combined.pathname;


		combined.pathname = path.join(base.pathname, combined.pathname);

Support path in the base url #23

Support path in the base url #23

Conversation

kaeedo commented Nov 21, 2017

msftclas commented Nov 21, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kaeedo commented Nov 22, 2017

bryanmacfarlane commented Dec 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanmacfarlane commented Dec 1, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bryanmacfarlane commented Dec 2, 2017

bryanmacfarlane commented Dec 2, 2017

Centro1993 commented Aug 10, 2020

msftclas commented Nov 21, 2017 •

edited

Loading