-
-
Notifications
You must be signed in to change notification settings - Fork 408
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 servant-client-jsaddle #1018
Conversation
Oh. This went completely under my radar. I'll check this tomorrow. |
Blip. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@phadej OK with this?
I'll add this to travis first, let's see then. |
|
||
testServer :: Server TestApi | ||
testServer x = do | ||
liftIO $ putStrLn "Hello tehre" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/tehre/there/?
Is there any way I can help move this along? |
@phadej did you make any progress with travis ci? |
Hello, Sorry for the delay! Both @phadej and I have been quite busy lately. I think we just need to add this new package to the |
@alpmestan I tried to do this here: roberth#1 |
@vaibhavsagar Your patch seems to be changing GHC versions, not just the list of packages to test? |
@alpmestan I think I misunderstood what you wanted me to do earlier. I've tried again now, can you have another look? |
aa2498d
to
85d6471
Compare
I accidentally force-pushed to the wrong branch, sorry about that! |
Is the diff against servant's master branch? Still seems like there are too many changes, wrt GHC versions etc. @phadej knows better though, so he may have more precise feedback. |
Yes, the diff is against |
Attempt to generate .travis.yml using haskell-ci
Apparently there's a conflict with our current travis script now, so the job won't run... Sorry about that, do you want to look into it? |
Fixed. |
OK, I now see:
|
I had to add: and an install plan with GHC-8.4.4 is found: (didn't try to build though) -- jsaddle -- Ubuntu packages:
|
I can't seem to reproduce this build failure locally. |
Ah, it looks like this is due to 4fab471. |
The 8.4 and 8.6 builds are choking on building |
We have faith in you @vaibhavsagar. |
Not requiring GUI toolkits is definitely a bonus
…Sent from my iPhone
On 8 Feb 2019, at 19.23, Alp Mestanogullari ***@***.***> wrote:
We have faith in you @vaibhavsagar.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added PR for PR: #1122
, http-media | ||
, http-types | ||
, jsaddle | ||
, jsaddle-dom |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a good reason we are using jsaddle-dom instead of ghcjs-dom?
It is usually preferred to use ghcjs-dom instead of jsaddle-dom directly, as it avoids some overhead when compiled with ghcjs. Also as other libraries usually depend on ghcjs-dom- jsaddle-dom does become an additional build dependency when using servant-reflex-jsaddle right now.
I am going to provide a PR for this PR, changing that dependency and fixing some imports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reason was portability. Looks like ghcjs-dom provides the same portability, so sgtm.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eskimor using ghcjs-dom
might also unblock the 8.4 and 8.6 builds, which are currently failing when trying to compile JSDOM.Types
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not, as ghcjs-dom has a similar module - and on ghc it is even the same.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(ghcjs-dom uses jsaddle-dom on ghc under the hood)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see.
This way we avoid the dependency on jsaddle-dom on ghcjs and have slightly better performance on ghcjs.
I did not run the tests though, as I have problems with some dependency.
Use ghcjs-dom
Make sure response gets handled even in case of exception.
I was able to build it with jsaddle-warp and updated 0.16: https://github.com/dredozubov/servant/tree/wip-servant-client-jsaddle-016 I've probably messed up the .travis.yml merge, but overall it's good news |
, case-insensitive >= 1.2.0.0 && < 1.3.0.0 | ||
, containers >= 0.5 && < 0.7 | ||
, exceptions >= 0.8 && < 0.11 | ||
, http-media |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why are version bounds ommitted for these deps?
Co-Authored-By: Herbert Valerio Riedel <hvr@gnu.org>
Btw, what's blocking this PR currently? Also, it seems like the |
This version of the patch requires xfvb and libwebkit2gtk-4.0-dev and other crap from CI. I recall there was a branch of this branch which doesn't need all of that, but it didn't emerge into PR. To be clear: I know very little about jsaddle, so cannot cleanup myself. This PR isn't such I could just press merge. |
I have a branch that I updated to use 0.16 https://github.com/dredozubov/servant/tree/wip-servant-client-jsaddle-016 It has a few rough edges: I may have screwed up the merge of .travis.yml(I haven't bothered to check) and I skipped the streaming part for the request handling. Nonetheless, it builds with jsaddle-warp and I've used it in an application. Should I force-push it here or create a PR for this PR? |
This is @roberths branch, I have no idea who can or cannot push there; but I wouldn't. |
@dredozubov just create a PR for the PR and ping me. |
Btw, I'd like the clean-history PR at the end. Dozens of commits with merge commits in between won't be merged. |
|
Contributions welcome :)
To do