Make an allow list of deep linkable routes #158
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbrandonw
commented
May 12, 2017
Library/Navigation.swift
Outdated
| @@ -260,6 +267,33 @@ private let routes: [String:(RouteParams) -> Decoded<Navigation>] = [ | |||
| "/users/:user_param/surveys/:survey_response_id": userSurvey | |||
| ] | |||
|
|
|||
| private let deepLinkRoutes: [String:(RouteParams) -> Decoded<Navigation>] = allRoutes.allValues( | |||
There was a problem hiding this comment.
this is the allow-list of routes we'll use for deep linking. I'm using this lil private helper Dictionary.allValues(from:) to pluck out a certain set of keys from the allRoutes dictionary.
open to other names!
There was a problem hiding this comment.
oh actually, using Hoogle I found this function:
restrictKeys :: Ord k => Map k a -> Set k -> Map k awhich is a better name and a better definition! maybe we should even add this to the prelude...
theginbin
approved these changes
May 12, 2017
| "/projects/:creator_param/:project_param/posts/:update_param/comments", | ||
| "/projects/:creator_param/:project_param/surveys/:survey_param", | ||
| "/users/:user_param/surveys/:survey_response_id" | ||
| ] |
There was a problem hiding this comment.
nice! only suggestion would be to put these keys into an enum since a string could potentially change in one of these places and not the other.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Right now our routing systems has a big ole list of URLs on Kickstarter that we want to recognize in a variety of situations, e.g. when inspecting requests in a web view, and when deep linking into the app.
Sometimes we want to use a different set of recognizable routes depending on the situation we are routing. In particular, when deep linking there is a smaller set of routes we want to recognize, and then anything we don't recognize can bump the user back to mobile safari.
That's what this PR does...
There are smarter ways to do this, and would love to try it out soon, but this will do the job and is well tested!