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

Allows Router to handle multiple prefix (Universal Links and Custom URL Scheme) in one object #35

Closed
wants to merge 7 commits into from

Conversation

freddi-kit
Copy link

@freddi-kit freddi-kit commented Jul 10, 2021

Goal

Allows Router to handle multiple prefixes (Universal Links and Custom URL Scheme) in one object

Issue

Related?: #31

Focused Problems

If developers want to use CrossRoad for universal URL and custom scheme url in them app, developers should prepare 2 router object, like,

let routerForCustomScheme = SimpleRouter(scheme: "pokedex")
let routerForUniversalScheme = SimpleRouter(url:  URL(string: "https://my-awesome-pokedex.com")!)

// 1. Registering patterns for both schemes
routerForCustomScheme.register ...
routerForUniversalScheme.register ...

// 2. Handling from provided URL by Crossroad user by checking prefix of URL string
if urlString.hasPrefix("https://my-awesome-pokedex.com") {
    routerForUniversalScheme.openIfPossible ...
} else if urlString.hasPrefix("pokedex://") {
    routerForCustomScheme.openIfPossible ...
}

I think that there are 2 issues which Crossroad users feel redundant.

  1. Registering patterns for both schemes by two object.
    • it produces duplicated code and sometimes makes it hard to maintain code. We cannot detect like: if one is obeying business logic correctly but another one is not.
  2. Handling from provided URL by Crossroad user by checking prefix of URL string
    • If users prepared 2 routers, they still have to handle comparing prefixes of passed URL by themselves.

Solution (My Change)

So I prepared a new initializer that allows using both schemes in one router object.

let router = SimpleRouter(scheme: "pokedex", url:  URL(string: "https://my-awesome-pokedex.com")!)

// 1. Registering patterns only once! (patterns must not have prefix)
router.register([
     (":pokemonName", { _ in ... }),
        ...
])

// 2. Handling both scheme in one object without checking prefix by ourselves!
router.openIfPossible(URL(string: "https://my-awesome-pokedex.com/pikachu")!) // Universal
router.openIfPossible(URL(string: "pokedex://pikachu")!) // Custom Scheme

Test

I already added a test that fulfills the rule of exiting tests but please tell me if other are missing

README.md Show resolved Hide resolved
@freddi-kit
Copy link
Author

freddi-kit commented Jul 10, 2021

I'm not supporting the patterns which has prefix.

let router = SimpleRouter(scheme: "pokedex", url:  URL(string: "https://my-awesome-pokedex.com")!)

// 1. Registering patterns only once! (patterns must not have prefix)
router.register([
     (":pokemonName", { _ in ... }), // ok
        ...
     ("pokedex://:pokemonName", { _ in ... }), // ng if you want to handle it as `pokedex://pikachu`
        ...
])

Please tell me if above pattern should be allowed but I'm thinking it is okay to not support it for multiple one. Here is a implementation https://github.com/freddi-kit/Crossroad/compare/multiple-router...freddi-kit:multiple-router-with-support-allcase?expand=1

@giginet
Copy link
Owner

giginet commented Oct 16, 2021

Hi freddi! 😄

I decided to rewrite inner logic completely to support multiple link sources.

I apologize it's difficult to apply your patch in spite of your efforts.

I'm working on the branch.
https://github.com/giginet/Crossroad/tree/2.0.0-alpha

I'll notify you of the progress of my refactoring.

@freddi-kit
Copy link
Author

No problem😊 let me close my PR and contribute in the future :)))

@freddi-kit freddi-kit closed this Oct 16, 2021
@freddi-kit freddi-kit deleted the multiple-router branch October 16, 2021 20:49
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

2 participants