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

Add map and path prefixing to targets #121

Merged
merged 2 commits into from Nov 11, 2020
Merged

Add map and path prefixing to targets #121

merged 2 commits into from Nov 11, 2020

Conversation

ghost
Copy link

@ghost ghost commented Nov 9, 2020

Hi again!

We're making heavy use of this library, and these additions would help us, if you think they make sense (happy to think again about the choice of "/~" as an operator name).

@anuragsoni
Copy link
Owner

Hi @Chattered ,

Thank you for making the proposal. The technical changes itself seem fine, but i'm curious about the usecase they intend to solve. If possible would you be able to give some examples of how your project intends to make use of these functions? I only ask because while /~ might be convenient in some cases, i'm not sure the burden of adding another infix operator might be worth it if its possible to achieve a similar behavior with the current set of operators. I'm thinking of something like:

let make_route p1 p2 = p1 / p2;;
let r1 = int / str;;
let r2 = s "foo" / s "bar";;
let r2 = make_route r2 r1 /? nil

On a similar note, the change to map seems technically correct, but I'm curious about how you intend to use it?

One way i reuse routes with the current api is by using thunks when defining the routes:

let r () = s "sum" / int / int /? nil;;

let sum_as_int = r () @--> fun a b -> a + b;;

let sum_as_string = r () @--> fun a b ->
Printf.sprintf "Sum of %d and %d = %d" a b (a + b);;

The thunk looks awkward indeed, and I think adding an operator to make working with that easier would be nice.

I'm mostly saying all this just to start a discussion and I hope you don't take this as me rejecting this proposal 😄

@ghost
Copy link
Author

ghost commented Nov 10, 2020

Hey @anuragsoni.

In our setup, we've got 400+ endpoints, each partially defined by a record which has a field for the target. We later attach handlers, and after that, we do some namespacing of the endpoints by prefixing various paths. I suppose we could replace the target field with one of type (('a,'b) path -> ('a,'b) path) -> ('c,'d) target which takes the prefix and produces the target. My concern there is that we're already getting bogged down by type variables, and I'm wary that such functions can do a lot more than just prefixing, and I'd rather have the assurance that prefixing is all that's happening. I think we could also use a 'a router router with the outer router using a wildcard to pass on the suffix to the inner router, though that's going to involve splitting and joining the suffix for each match?

The mapping is more of an issue, just because of the way we've decomposed this application. We've got targets like (int -> handler, 'a) target and (int -> string -> handler, 'a) target with concrete handlers, and then in a routing module, we want to transform these handlers before building our actual router. I think we can get rid of the decomposition and just inline the mapping, but I'd be a bit sad about that.

My personal philosophy is that if I can see how a type constructor is a functor (in the mathematical sense), then it's worth including the map function, and so allow for the sort of decomposition we're doing. Admittedly, I might be coming from a Haskell bias here, where a Functor instance doesn't have any real impact on the size of an interface.

Thanks for the quick feedback!

@anuragsoni
Copy link
Owner

anuragsoni commented Nov 11, 2020

Thanks for the insight @Chattered

My concern there is that we're already getting bogged down by type variables, and I'm wary that such functions can do a lot more than just prefixing, and I'd rather have the assurance that prefixing is all that's happening.

I think this is enough of a justification that supporting this in the library is justified :) Thanks for providing the examples of how you use it!

The mapping is more of an issue, just because of the way we've decomposed this application. We've got targets like (int -> handler, 'a) target and (int -> string -> handler, 'a) target with concrete handlers, and then in a routing module, we want to transform these handlers before building our actual router. I think we can get rid of the decomposition and just inline the mapping, but I'd be a bit sad about that.

This makes sense. I don't see any problem with supporting a map operation in the library.

happy to think again about the choice of "/~" as an operator name

I'm going to merge this PR in its current state, but I'll probably spend some time in thinking about the operators to see if we can come up with something different that might be easier to read.

In our setup, we've got 400+ endpoints, each partially defined by a record which has a field for the target. We later attach handlers, and after that, we do some namespacing of the endpoints by prefixing various paths.

Thanks for sharing this. I really appreciate getting to learn about your usage pattern as this sounds bigger than most applications i've heard of that use routes.

@anuragsoni anuragsoni merged commit 62bba51 into anuragsoni:master Nov 11, 2020
@ghost
Copy link
Author

ghost commented Nov 11, 2020

Awesome! Thanks again.

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

1 participant