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

Update for 0.19 #45

Closed
wants to merge 3 commits into from
Closed

Update for 0.19 #45

wants to merge 3 commits into from

Conversation

mattjbray
Copy link
Collaborator

Thanks @stoeffel for the initial work on this.

Needs some changes in elm-export for 0.19.

@FranklinChen
Copy link

Any more on this?

@FranklinChen
Copy link

Should I pick up where this was left off since it hasn't been finished yet?

@mattjbray
Copy link
Collaborator Author

@FranklinChen I think this is all that is needed in servant-elm - in theory this PR is ready to go. But it looks like elm-export needs some changes for 0.19 - e.g. decode has been removed from elm-json-decode-pipeline. There might be others.

@domenkozar
Copy link
Collaborator

@mattjbray maybe we should revisit the decision of keeping elm-export and move to elm-bridge, which seems superior in terms of features and it's already Elm 0.19 ready. I think supporting two backends wouldn't yield many benefits and would have maintenance overhead.

My 2c, thanks for this PR. There is some work on Elm 0.19 in this branch: https://github.com/matsubara0507/elm-export/commits/elm-0.19

@domenkozar
Copy link
Collaborator

@mattjbray this PR breaks my codebase because it doesn't support newtypes anymore for encoding parts of the url. Could newtypes around strings be easily supported or maybe allow to manually define toString for them.

@domenkozar
Copy link
Collaborator

Something like http://blog.stermon.com/articles/2018/04/09/elm-stringeable-types-library-for-elm-019.html would allow compile-time check and one would have to implement those manually.

On second thought, having those inpuits as String would also work for now.

@domenkozar domenkozar mentioned this pull request Oct 2, 2018
@domenkozar
Copy link
Collaborator

Would also cover #35

@FranklinChen
Copy link

What's the current status of elm-export use?

@domenkozar
Copy link
Collaborator

elm-export PR: krisajenkins/elm-export#54

@saurabhnanda
Copy link

Apologies for butting into this PR.

I'm working on API code-gen for Elm 0.19 based off elm-bridge and am looking at servant-elm for inspiration. If I look at the Elm docs for Http it seems that it is no longer possible to return an Http.Response a from any function in that module. You are forced to return a Cmd Msg. Have I understood this correctly?

I tried loading the one of the generated sample Elm file from this branch - https://github.com/mattjbray/servant-elm/blob/f665ddd5cefac26b3e56981f3516772d3987a6f3/examples/books/elm/Generated/BooksApi.elm - and got a bunch of these errors:

> import BooksApi
-- NAMING ERROR ----------------------------------------------- src/BooksApi.elm

I cannot find a `Http.Request` type:

70| getBooksByBookId : Int -> Http.Request (Book)
                              ^^^^^^^^^^^^
The `Http` module does not expose a `Request` type. These names seem close
though:

    Http.Expect
    Http.Header
    Http.Progress
    Http.Response

Hint: Read <https://elm-lang.org/0.19.0/imports> to see how `import`
declarations work in Elm.

Does this mean that ElmOptions will have to evolve to include an "API-endpoint <> Elm-message mapping"? Another solution could be to have a Servant-level type-combinator, say ElmMessage to achieve the same, eg.:

type Api = "books" :> Capture "bookId" Int :> ElmMessage "BookReceived" :> Get '[JSON] Book

If I have understood this correctly, isn't this a big spanner thrown by Elm 0.19 wrt API code-generation?

@saurabhnanda
Copy link

Another solution could be to generate functions that take a Msg as one of the arguments, eg:

type Msg = BookReceived (Result Http.Error Book)

getBookById : Msg -> Int -> Cmd Msg
getBookById msg bookId = Http.get
  { url = "/books/" ++ (String.fromInt bookId)
  , expect = Http.expectJson msg Book.decode
  }

This might not be a bad idea after all. One might want to emit different messages for different call-sites of getBooksById. This brings down the "alarm level" from my side for sure 😄

@domenkozar
Copy link
Collaborator

I have hit the same issue and it's possible to use generics, but as soon as there is a manual instance in Haskell, something like http://blog.stermon.com/articles/2018/04/09/elm-stringeable-types-library-for-elm-019.html would work.

@saurabhnanda
Copy link

@domenkozar didn't understand your comment completely. Does #45 (comment) not solve the problem? I've taken a stab at a POC at agrafix/elm-bridge#40 (comment)

@domenkozar
Copy link
Collaborator

@saurabhnanda sorry, wrong thread :)

@AesaKamar
Copy link

Were there any additional conclusions that've come out of this regarding next steps to be taken to support Elm 0.19?

@szg251
Copy link
Contributor

szg251 commented Mar 14, 2019

I forked this repo and made it compatible with 0.19 but it needs elm-export to be updated too.
I think this PR should be alright, but seems to be abandoned.

Here's my repo:
https://github.com/gege251/servant-elm
Since the elm-export package is referenced by a git url I'm not sure if I should create a new PR.

@k-bx
Copy link
Collaborator

k-bx commented Mar 20, 2019

Reposting here since this place is more relevant than an elm-export PR:

I've made another fork. I've replaced elm-export with elm-bridge, but overall code quality is pretty alpha.

https://github.com/k-bx/servant-elm

Example CRUD I'm using as a basis: https://github.com/k-bx/servant-elm-generically

@domenkozar
Copy link
Collaborator

Maybe we could maintain this at https://github.com/haskell-servant/servant-elm as a community?

@k-bx
Copy link
Collaborator

k-bx commented Mar 20, 2019

Would be happy to, would you kick-off the process? Also, any thoughts on moving to elm-bridge?

@domenkozar
Copy link
Collaborator

@k-bx see #43 - I'm for it. Since 0.19 breaks backwards compatibility it seems like a good time to use elm-bridge.

@alpmestan @mattjbray - what do you think about moving servant-elm to haskell-servant?

@alpmestan
Copy link

I personally wouldn't mind, as long as some people volunteer to maintain it, since @phadej and I have quite a lot on our plates already =)

@domenkozar
Copy link
Collaborator

I can help with maintenance, but we'd need another 1-2 co-maintainers.

@k-bx
Copy link
Collaborator

k-bx commented Mar 21, 2019

I could definitely co-maintain. I've started using my fork in my side-project yesterday, while it already unveiled few substantial problems, it's working ok with few hacks, and I later plan to use it on my main work project (which is much bigger and needs better lib quality).

@k-bx
Copy link
Collaborator

k-bx commented Mar 21, 2019

FYI, I've also just migrated my open-source side-project from pure elm-bridge to my fork to generate the Api.elm https://gitlab.com/k-bx/meetup/commit/a91c6fcc3ee6a8c900a265fc09a3cf99eecbbeb9

I think it could be a good project to use for our "real-world usage" purposes as we go. Website is at https://meetup.events, code at https://gitlab.com/k-bx/meetup

@mattjbray
Copy link
Collaborator Author

@domenkozar I'm happy to transfer this repo to haskell-servant. In the meantime I've added you to this repo. Unfortunately I've not had a lot of Haskell projects recently so I haven't been giving servant-elm the attention it deserves :)

Feel free to take the 0.19 update and migration to elm-bridge forward.

@k-bx
Copy link
Collaborator

k-bx commented Mar 21, 2019

@mattjbray more importantely, please add @domenkozar as a library maintainer on Hackage (feel free to add me as well, my nick is k_bx)

@mattjbray
Copy link
Collaborator Author

@k-bx Done.

@domenkozar @alpmestan let me know about moving the repo to haskell-servant.

@alpmestan
Copy link

Where is it again that we can move projects to other orgs, isn't it in this project's settings?

Let me know if there's anything I need to do on the "haskell-servant side".

@szg251
Copy link
Contributor

szg251 commented Mar 21, 2019

@domenkozar I am also happy to help if I can.

@domenkozar
Copy link
Collaborator

Thanks all :) I'll add @mattjbray, @gege251, @k-bx and me as maintainers and to the README once we do the transfer.

@domenkozar
Copy link
Collaborator

Superseded by #49

@domenkozar domenkozar closed this Jun 3, 2019
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.

9 participants