Skip to content

[RFC] Add TypeRep of api end-point to Foreign.Req. #630

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

fisx
Copy link
Member

@fisx fisx commented Oct 31, 2016

Not ready for merge, just interested in your thoughts on this. If you're short on time feel free to ignore or close.

Motivation: I want to write the api type of each end-point into a comment with the js client functions:

// :> Symbol * "r" (:> Symbol * "vote" (:> * * (Capture * "vote" PatchKey) (:> * * (ReqBody * (: * JSON []) IdeaVoteValue) (Verb StdMethod * POST 200 (: * JSON []) PatchVote))))
var postRVoteByVote = function(vote, body, onSuccess, onError)
{
  $.ajax(
    { url: '/r/vote/' + encodeURIComponent(vote) + ''
[...]

If you like this change, I will rebase this to current master, apply the changes to the other servant-js targets (currently there is only jquery), run the tests (should i add any for this?), and consider any other changes you may require.

Thanks!

@fisx fisx force-pushed the document-generated-js branch from 9af5dda to 2a4efaa Compare October 31, 2016 11:48
@fisx
Copy link
Member Author

fisx commented Oct 31, 2016

argh. just noticed this isn't working quite yet. anyway, interested in hearing how you like the idea! (-:

@fisx
Copy link
Member Author

fisx commented Oct 31, 2016

[somewhat fixed it]
[updated description above]

open problem: api types "only_end_point" :> Get '[JSON] Int has the default type (because it doesn't hit the :<|> instance), and AuthCombinator :> (route1 :<|> route2) doesn't mention the AuthCombinator in the types (because it hits the :<|> instance before it gets to that).

one solution would be to introduce an explicit type that steers update of reqApiType, and that we need to wrap each end-point in. this would have the benefit that the change to servant, servant-js would be minimal (only change the Req type), and the drawback that it would uglyfy the api types.

@phadej
Copy link
Contributor

phadej commented May 14, 2017

@haskell-servant/maintainers can someone comment on this, servant-foreign code is quite foreign to me.

@alpmestan
Copy link
Contributor

If @fisx still wants this, I would not mind. Would this break packages depending on -foreign? If it would, we might want to involve the maintainers of those packages. @fisx would you mind leading that discussion, if you still want this patch to be merged?

@fisx
Copy link
Member Author

fisx commented May 15, 2017

I don't need this right now, but I do not want it to go to waste. I'll try to get it rebased by the end of the week.

@tchoutri
Copy link
Contributor

@fisx Hi, any plans for this PR? Otherwise can I close it?

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

Successfully merging this pull request may close these issues.

4 participants