microsoft / typed-rest-client

Node Rest and Http Clients with typings for use with TypeScript
Other
675 stars 118 forks source link

Removes the setting of the default port if one isn't specified. #67

Closed etiago closed 6 years ago

etiago commented 6 years ago

This fixes behavior where a baseUrl isn't defined and we expect the client to fetch based on the current root url whilst running in the browser.

Referenced in issue #66 .

etiago commented 6 years ago

The idea is to not take a default port if one isn't specified. When running in the browser we simply fallback to whatever the original URL was, whereas currently, if a baseUrl wasn't specified and a relative URL is used in the requests, we end up with illegal URLs such as https://www.foobar.com:3000/:80/bla due to adding the :80 as a default port.

etiago commented 6 years ago

Unfortunately when I visit https://cla.opensource.microsoft.com/ I don't see any way for signing the CLA digitally... how can I sign it in order to get this merged?

stephenmichaelf commented 6 years ago

It looks like the signing requests were met based on this PR.

stephenmichaelf commented 6 years ago

I am a bit confused, you're saying this is just in the case where there is no base url and it's a relative path? Could you add some unit tests for all scenarios (base/relative and port/no port)? We have to be careful with compat as current users of the lib could be relying on the default port.

etiago commented 6 years ago

I've had a look at the tests and I don't think I could very easily write a test case for this, as this is an issue specifically when using the library in the browser (possibly this should be detected and have browser-specific behavior). When using the library in a Node application it would make sense to always specify a baseURL and rely on relative paths per request or require absolute paths on every request.

However, when using the library in a client-side application, say, in a React app, this is slightly different. Not having a baseURL does not imply requiring absolute paths per request. This is because of how the underlying Javascript will handle a request to /foo to actually be a request to www.whatever-website-i-am-on.com/foo.

This can be shown with two examples:

let foo = new rm.RestClient('foobar');

let result = await foo.get('https://npm.runkit.com/'); result.statusCode

let result = await foo.get('/'); result.statusCode



It will complain that the last request can't connect to `127.0.0.1:80` as it's trying to connect to the default server and port (127.0.0.1 and 80 respectively).

- Secondly, try the following Fiddle https://jsfiddle.net/6r128a4v/1/

This is how HTTP libraries normally behave when running client-side in a browser. Requests to `/` take the current address as base.