Fix base url #9
Fix base url #9
Conversation
|
@cyrusaf the only difference between v5 and v6 really is that v5 has the |
|
Fix published https://rubygems.org/gems/twirp/versions/0.5.1 |
| "/#{service_full_name}/#{rpc_method}" | ||
| def make_http_request(conn, service_full_name, rpc_method, content_type, body) | ||
| conn.post do |r| | ||
| r.url "#{service_full_name}/#{rpc_method}" |
There was a problem hiding this comment.
maybe use the class-level method Client#rpc_name here? Since it removes the usage of magic strings across the codebase.
There was a problem hiding this comment.
You mean Client.rpc_path method? I removed it because it was only used once in this place, and I think using the string literal makes it a little more readable (e.g. is clear that there is no trailing slash). If we find the need to add the Client.rpc_path method back then yes it will make sense to use it from here as well.
There was a problem hiding this comment.
makes sense, and yes excuse my typo.
Using a base_url with a path prefix was not working as expected: the path prefix was being ignored. This was reported in this PR: #8, but the problem was a missunderstanding of the twirp protocol v6 (still unpublished) and also a bug in the code that was preventing the right behavior.
The issue is that the client was using url:
"/#{service_full_name}/..."(with a leading slash), which for Faraday means to use an absolute URL, thus ignoring the prefix (see issue lostisland/faraday#293).The fix was just to remove the leading slash in the url: `"#{service_full_name}/..." to make it a relative path, thus respecting the path prefix.
In addition, the example app and README were changed to represent the common case of using a
/twirpprefix in the URL.