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

QueryParam's order #119

Open
farzadshbfn opened this issue Mar 6, 2018 · 10 comments
Open

QueryParam's order #119

farzadshbfn opened this issue Mar 6, 2018 · 10 comments

Comments

@farzadshbfn
Copy link

farzadshbfn commented Mar 6, 2018

I know this is really stupid, but some apps, do not support QueryParameters like they should, and only support QueryItems in custom order. (like Maps.Me)

Any suggestions on how to fix this problem?
I was trying to add:

enum Query {
    case parameters([String:String])
    case items([URLQueryItem])

    var queryItems: [URLQueryItem] {
        switch self {
            case .parameters(let params): return params.map { URLQueryItem(name: $0, value: $1) }
            case .items(let queryItems): return queryItems
        }
    }
}

And use Query as an input, But since Path's queryParameters is variable and public, it'll definitely break backward compatibility.
Any suggestions?

@Maryom
Copy link
Contributor

Maryom commented Mar 8, 2018

Could you please provide the error you got, when you tried to use Appz to add this application ?

@farzadshbfn
Copy link
Author

It's not the Appz actually, Inside the Path, u're using a [String: String] for queryParameters, and it sounds fine because that's how queryParameters were meant to be, but some apps (as i said like Maps.Me), did not implement their deeplinking correctly, so the Order of queryParameters are important. using a Dictionary [String: String], does not guarantee the order of queryParams.

@Mazyod
Copy link
Member

Mazyod commented Mar 8, 2018

Thanks for reporting this!
I thought of this simple, non-intrusive solution. Would love to hear your thoughts:

public enum QuerySortStrategy {
    case none
    case array([String])
    case callback((String, String) -> Bool)
}


public struct Path {
    
    public var pathComponents = [String]()
    public var queryParameters = [String:String]()
    public var querySortStrategy = QuerySortStrategy.none
    
    public var queryItems: [URLQueryItem] {
        let items = queryParameters.map {
            URLQueryItem(name: $0, value: $1)
        }
        
        switch querySortStrategy {
        case .none:
            return items
        case let .array(keys):
            return items.sorted {
                (keys.index(of: $0.name) ?? Int.max) < (keys.index(of: $1.name) ?? Int.max)
            }
        case let .callback(callback):
            return items.sorted {
                return callback($0.name, $1.name)
            }
        }
    }
    ...
}

// tests
func testQuerySortStrategies() {
    var path = self.path
    // validate initial order assumption
    XCTAssertEqual(path.queryItems.map { $0.name }, ["1", "11"])
    // array strategry
    path.querySortStrategy = .array(["11", "1"])
    XCTAssertEqual(path.queryItems.map { $0.name }, ["11", "1"])
    // closure strategy
    path.querySortStrategy = .callback({
        $0.count > $1.count
    })
    XCTAssertEqual(path.queryItems.map { $0.name }, ["11", "1"])
}

@farzadshbfn
Copy link
Author

It will solve the problem, but me myself, i'm not a fan of this way, since it is not error-prune. consider the cases when most of people forget to insert a Key inside .array or in the queryParameters or ...

@Mazyod
Copy link
Member

Mazyod commented Mar 9, 2018

Having a [URLQueryItem] instead of [String:String] is the approach I'd chose as well, but of course, we need to be mindful of backward compatibility.

So .. This approach properly isolates the sorting strategy from the key/value pair definition. If a user forgets to give the proper order in their sorting strategy, they might make the same mistake in arranging them within an array, so it is a problem present in both cases.

However, they might omit a key from the array on purpose, which is a feature, that means you don't care about the position of that key, so just throw it at the end.

Hence, I think this solution is not ideal, but is also free from any major flaws.

@Mazyod
Copy link
Member

Mazyod commented Mar 9, 2018

another possibly way is to allow an optional [URLQueryItem], and simply merge the two. Would that work better for you?

@farzadshbfn
Copy link
Author

Well, I imported the Appz code directly into my code and solved my problem with the way i proposed (using an enum Query as an input).
I'm not in a hurry anymore, let's find the best solution possible ^_^.
what about using DictionaryLiteral? it will still break backward-compatibilty, but it won't cost much to fix the problems (for anyone which did use queryParameters directly). And it's also ordered...

@Maryom
Copy link
Contributor

Maryom commented Mar 10, 2018

Well, I remember we didn’t add Microsoft apps because they didn’t work with Appz. So, I have a suggestion if you don’t mind: I think the best thing to do is: to find a solution that works for both of this case and Microsoft apps.

@farzadshbfn
Copy link
Author

What's their problem?

@Maryom
Copy link
Contributor

Maryom commented Mar 10, 2018

Please check #30

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants