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

Feature: location payload #34

Merged
merged 13 commits into from
Aug 24, 2016
Merged

Feature: location payload #34

merged 13 commits into from
Aug 24, 2016

Conversation

vadymmarkov
Copy link
Contributor

@zenangst @onmyway133 @RamonGilabert

  1. We had some discussions about type-safety and magic strings in fragments. We can't really use generics just for optional function parameters. And if we conform to Mappable like in Feature: mappable meta Brick#12 it adds Tailor as a dependency and could lead to many small struct types in the project codebase. My solution here is to add a new property on Location:
public let payload: Any?

Since it's Any we can pass whatever we want and then cast it to the type that we expect:

struct User {
  let firstName: String
  let lastName: String
}

let user = User(firstName: "foo", lastName: "bar")

guard let location = Compass.parse(url, payload: user) else {
  return
}

// ...

if let user = location.payload as? User {
  print("\(user.firstName) \(user.lastName)")
}
  1. If you like new payload property, do we need fragments at all? Because you can set [String: AnyObject] dictionary as payload. The only disadvantage is that you have to cast it later:
guard let location = Compass.parse(url, payload: ["foo": "bar"]) else {
  return
}

// ...

if let payload = location.payload as? [String: String] {
  print(payload["foo"])
}

What do you think?
  1. From now we can throw errors in Routable that will be caught in Router and handled by a new type called ErrorRoutable. It means we can present some error screen when the route is not found or arguments are invalid.
class ErrorRoute: ErrorRoutable {
  var error: ErrorType?

  func handle(routeError: ErrorType, from currentController: Controller) {
    error = routeError
  }
}

class ThrowableRoute: Routable {
  func navigate(to location: Location, from currentController: Controller) throws {
    throw RouteError.InvalidArguments(location)
  }
}

let router = Router()
let errorRoute = ErrorRoute()

router.errorRoute = errorRoute // errorRoute is an optional var
router.routes["throw"] = ThrowableRoute()
router.navigate(to: Location(path: "throw"), from: UIViewController())

print(errorRoute.error) // RouteError.InvalidArguments

@zenangst
Copy link
Contributor

  1. Magic strings bad! This PR good!
  2. Well you kind of have to type-cast things that are optional regardless so I don't really see the harm in that.
  3. Oooh, errors! You could hook this up with RavenSwift and so on for debugging purposes. And display a message to the user, this is great!

@vadymmarkov
Copy link
Contributor Author

@zenangst

  1. Does it mean you agree with removing fragments in favour of payload?
  2. Is payload a good name for that?

@zenangst
Copy link
Contributor

Yeah payload works, I kinda liked fragments but it is a bit confusing as it is an Android term.... #thanksandroid.

I'm totally for payload 😁

@vadymmarkov
Copy link
Contributor Author

@zenangst fragments field is officially removed. So I think this PR is ready to merge. And yeah, it's a breaking change again 😄

@zenangst
Copy link
Contributor

rick-morty-too-much

@onmyway133
Copy link
Contributor

This is good 🎉
But ErrorRoutable, ThrowableRoute terms sound not very intuitive

@vadymmarkov
Copy link
Contributor Author

@onmyway133 ThrowableRoute is just a name in my example. ErrorRoutable is a part of public API though. Feel free to suggest a better name 😄

@zenangst
Copy link
Contributor

@onmyway133 any suggestions? otherwise I'll just go ahead and merge this bad-boi

@onmyway133
Copy link
Contributor

@zenangst I think you can go ahead

@zenangst zenangst merged commit 5897840 into master Aug 24, 2016
@zenangst zenangst deleted the feature/payload branch August 24, 2016 08:45
@zenangst
Copy link
Contributor

AAAA YEAH!

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

3 participants