search

truqu / elm-oauth2

OAuth 2.0 client-side utils in Elm
MIT License
81 stars 29 forks source link

Upgrade to elm/http version 2.0.0 #17

Closed billstclair closed 5 years ago

billstclair commented 5 years ago

This is similar to @michaelwebb76's PR #16, but it provides the body with a BadStatus response, so that the AuthorizationCode request can parse the returned JSON for the error.

I have tested the AuthorizationCode branch in billstclair/elm-oauth-middleware.

PR #16 is simpler, and should be sufficient for other than AuthorizationCode errors. If you choose that one, I'll probably make my own copy of the AuthorizationCode code in elm-oauth-middleware.

billstclair commented 5 years ago

Note that I haven't tested the examples. They build, but I didn't try them.

KtorZ commented 5 years ago

I have actually started to integrate the work from #17 on a local branch. Doing some modifications at the same time. I'll also have a look at yours @billstclair and take the best of both worlds.

Thanks for submitting this :heart:

michaelwebb76 commented 5 years ago

@KtorZ I would love this to be fixed - it's holding up one of our internal projects. Is there anything I can do to help move it along?

billstclair commented 5 years ago

I'm not surprised that @KtorZ chose the other pull request. Closing this one. WIll adjust my client code accordingly.

billstclair commented 5 years ago

The only changes I had to make to billstclair/elm-oauth-middleware, other than the HTTP 2.x changes I already made, were to the server-side code. Mine is probably the only package using that, so I can heartily agree with the choice of the simpler HTTP 2.0 upgrade.

KtorZ commented 5 years ago

@billstclair Apologizes for the time it took me to get back to you. Yet, indeed, I reviewed both PRs and, made a few changes to the one from @michaelwebb76 before merging it.

I first had a hard-time understanding why in your PR you were redefining the HTTP error types and re-read carefully your description above. Indeed, I don't understand why the native elm http library doesn't include the body for the BadStatus branch when using the basic expect*** helper. I guess there's a valid design decision behind this but I fail at grasping it here :/ ...

Having said that, including a superset of the HTTP error within the OAuth library feels wrong and feels like accommodating here for flaws in the built-in http package. That's not something I am really willing to do, hence why I didn't merge this. If you can work around this, then it's all good and I'd actually be really keen to have a look at how to maybe include that in the troubleshooting section of the README :+1:

billstclair commented 5 years ago

Working around my kluge on the package as you updated it ended up being very easy, and only required a small change to the receipt of the token from the billstclair/elm-oauth-middleware server, something that nobody but me may ever do. I applaud you for discarding my proposal.