RuiAAPeres / Reactor

Powering your RAC architecture
MIT License
184 stars 11 forks source link

NSURLComponents path assignment deletes previous path #24

Closed portellaa closed 8 years ago

portellaa commented 8 years ago

Hi,

In first place, congrats Reactor is awesome :)

Well, sometimes based on the environment, my baseURL has a path after the host, i mean for example, staging.api.com/v2/ and for the prod environment, api.com/

When the NSURLComponents inits, in the staging url it assigns v2 to the path and for example, when i create a Resource to GET popcorns, i do Resource(path: "/popcorns", method: .GET) it will build an URL like staging.api.com/popcorns.

I found that Resource assigns the path to the generated NSURLComponents which will delete the previous path value.

My suggestion is that we should append the new value, instead of assign (if any exists).

So in the Resource class replace components?.path = path with:

if let componentsPath = components?.path {
    components?.path = componentsPath
        .stringByAppendingString(path)
        .stringByReplacingOccurrencesOfString("//", withString: "/")
 }

Let me know if you agree and want that i create a PR with this.

Cheers 🍻

RuiAAPeres commented 8 years ago

We had this issue in our own project as well, until @Krisiacik realized that the way we were doing was fundamentally broken. We would mix things like the host and path and assume that has being the host. We were also concatenating the different parts together (host, path and query) manually.

Eventually it was decided to go with the NSURLComponents approach, so it formalizes the way we create a Resource.

I found that Resource assigns the path to the generated NSURLComponents which will delete the previous path value.

I think the problem, now that I am looking at it, is that we ask for a baseURL, but instead what we should ask for, is a protocol (http, https, etc) + host (google) + top host (com). In your case, because you are passing staging.api.com/v2/ and /v2/ is part of the path, it gets removed.

Looking at your example, what you would really want here:

Resource(path: "/popcorns", method: .GET) is in fact Resource(path: "/v2/popcorns", method: .GET).

I would prefer to create a new method (similar to func toRequest(baseURL: NSURL) -> NSURLRequest), instead of hard coding the rule you purposed.

May I ask how are you passing this path value around in your architecture?

portellaa commented 8 years ago

Currently i have a PathBuilder, which builds the path based on the environment, so in the end i send /v2/popcors to the Resource.

My suggestion was to somehow improve how the URL is built, and support this because it's quite usual to have api endpoints which has something more after the top host. Well at least it would be great if Reactor could have anotherResource with something different of a path which is appended to the NSURLComponents

RuiAAPeres commented 8 years ago

Currently i have a PathBuilder, which builds the path based on the environment, so in the end i send /v2/popcors to the Resource.

Couldn't you build your domain the same way?