Add bundle parameter to route #207
Conversation
Source/Visit/VisitProposal.swift
Outdated
| public init(url: URL, | ||
| options: VisitOptions, | ||
| properties: PathProperties = [:], | ||
| bundle: [String: Any]? = nil) { |
There was a problem hiding this comment.
Nice, this will be great to have. If bundle feels like odd language for the platform, feel free to rename. Android uses bundle since it's a platform convention. On the destination side, when you extract the contents of a bundle, you use an arguments property to extract the key/value pairs. Would arguments be a better name here than you can use? It might make the intent more clear on the navigating side and the destination side.
There was a problem hiding this comment.
What actually is the bundle here?
There was a problem hiding this comment.
If bundle feels like odd language for the platform, feel free to rename.
It doesn't feel particularly odd. We could certainly rename it to arguments or context or something similar. While I like how it's consistent with Android naming, I don't feel too strongly about it.
What actually is the bundle here?
So, for example, in HEY we have an import contacts flow that's something like this:
- Present contacts from the user's phone (native:
CNContactPickerViewController) - When user selects contacts and taps done, move to an upload progress screen (native: e.g.
UploadContactsViewcontroller)
There's no easy path to move between screen 1 and 2 using TurboNavigator since screen 2 requires the contacts selected from screen 1. So we'd need to tightly couple 1 and 2 (defeats the purpose of having the path config) or pass the contacts some other way (not super clean).
Jay and I discussed this and Android has an easy way of doing this which is passing along the data required for setting up the screen alongside the route.
In the scenario above, we'd call something like route(to: "/native/contacts/upload", bundle: ["contacts" : selectedContacts]). The bundle becomes a part of the VisitProposal so when the client needs to resolve the proposal in handle(proposal: VisitProposal) they have all the data needed to setup the new client.
There was a problem hiding this comment.
Ohhh! Very interesting. Thanks for the explanation.
My vote is context. The term "bundle" is a little loaded on iOS in my opinion, since it could mean the actual application bundle itself.
There was a problem hiding this comment.
Ah! Sure, totally missed that. I'll rename to context. 👍🏻
There was a problem hiding this comment.
I don't think we should use context, since that conflicts with the existing first-class context: "default" and context: "modal" setup in the path config. Let's find something else that makes it clear how its intended to be used. I'm also fine renaming the Android one in the future if we find a good name.
There was a problem hiding this comment.
Good point too! I'll try to find a good name and report back.
There was a problem hiding this comment.
I like this idea!
Regarding naming, what about parameters or arguments?
There was a problem hiding this comment.
parameters ended up being the winner. Thanks everyone!
| @@ -42,10 +42,11 @@ public class TurboNavigator { | |||
| /// Convenience function to routing a proposal directly. | |||
| /// | |||
| /// - Parameter url: the URL to visit | |||
| public func route(_ url: URL) { | |||
| /// - Parameter bundle: provide context relevant to `url` | |||
| public func route(_ url: URL, bundle: [String: Any]? = nil) { | |||
There was a problem hiding this comment.
I'm wondering if we should go with Any? instead of [String: Any]?. This would allow to directly pass an object, instead of providing a key value pair.
There was a problem hiding this comment.
I think keeping this makes more sense as it maintains parity with Android.
Backfills turbo-ios PR #207: hotwired/turbo-ios#207
Backfillls turbo-ios PRs: * hotwired/turbo-ios#207 * hotwired/turbo-ios#209
When routing, sometimes it's necessary to pass along information relevant to the URL. For example, we may route to a URL that ends up being a native controller and the native controller requires additional data for setting up.
Android has something similar when navigating.