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

When a route matches multiple handlers, make it FIFO. #115

Open
Kalzem opened this issue Nov 6, 2018 · 3 comments
Open

When a route matches multiple handlers, make it FIFO. #115

Kalzem opened this issue Nov 6, 2018 · 3 comments

Comments

@Kalzem
Copy link

Kalzem commented Nov 6, 2018

Let's imagine the handlers

navigator.handle("app://services/defaultService") // n°1
navigator.handle("app://services/hidden/<string:service_id>") // n°2
navigator.handle("app://services/<string:service_id>") // n°3

If I pass the route app://services/github, then only handler n°3 will be triggered.
But if I pass the route app://services/defaultService, technically handlers n°1 and n°3 should trigger.

And it's where the problem starts: Because URLNavigator relies on a dictionary to save all the handlers, there is no order guaranteed.

open func handler(for url: URLConvertible, context: Any?) -> URLOpenHandler? {
    // handlerFactories is a dictionary, the keys are not guaranteed order of declaration.
    let urlPatterns = Array(self.handlerFactories.keys) 
    guard let match = self.matcher.match(url, from: urlPatterns) else { return nil }
    guard let handler = self.handlerFactories[match.pattern] else { return nil }
    return { handler(url, match.values, context) }
}

I suggest to implement an OrderedDictionary to save all the handlers and to follow a FIFO logic for the matchers.

Here is an example of an ordered dictionary: https://gist.github.com/BabyAzerty/2efda8bcef7a4286b5dc62770ea5aab5

It wouldn't change any URLNavigator code beside the declaration of the handlers to:

private var handlerFactories = OrderedDictionary<URLPattern, URLOpenHandlerFactory>()
@YuanzhengWang
Copy link

I met, too. This is very important.

@strangeliu
Copy link

I met, too. This is very important. :)

@devxoul
Copy link
Owner

devxoul commented Sep 7, 2019

I think the matcher should not be dependent on the order but how many paths the url matches. #120 improves matching algorithm. So app://services/github will only match with n°3 and app://services/defaultService will only match with n°1.

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

No branches or pull requests

4 participants