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

Only consume body when http code doesn't cause immediate rejection #13

Merged
merged 2 commits into from
Nov 26, 2019
Merged

Only consume body when http code doesn't cause immediate rejection #13

merged 2 commits into from
Nov 26, 2019

Conversation

WalternativE
Copy link
Contributor

  • To give consuming clients the chance to parse the body themselves for
    non-success status codes. This is especially important to extract
    additional information about errors encoded in the response bodies.

@MangelMaxime
Copy link
Contributor

Hello @WalternativE,

I am not sure about these changes because if you have an error it will still be captured by |> Promise.catch (NetworkError >> Error) when using tryFetchAs. And so result in an Error (NetworkError (Error FetchFailed)).

Can you please add tests to see what you want to achieve?

@WalternativE
Copy link
Contributor Author

I added a test for the scenario I had in mind. Maybe this makes the situation clearer. My problem is that currently the resolve function always reads the text of the fetch response. If the status code is a non-success it packs the response in an Error(FetchFailed response). I can get this response and try to parse the information I need but unfortunately I cannot re-read the body. All the information (and it's not unconventional to return very specific error information in many APIs) in the body is lost to me. That's why I proposed the changes you see in this PR.

Might very well be that there is a way to re-read the body which I didn't consider. Did I overlook something?

@MangelMaxime MangelMaxime merged commit 643f330 into thoth-org:vNext Nov 26, 2019
@MangelMaxime
Copy link
Contributor

Ok, I see.

I had well understood your case but I didn't know we couldn't read several times the body. So here, you are delaying it until last time.

I also originally misunderstood what Promise.lift does.

Because your test is passing and all the others are still green I will release a new beta version.

@MangelMaxime
Copy link
Contributor

Thoth.Fetch 2.0.0-beta-002 has been released.

@WalternativE
Copy link
Contributor Author

Thanks, Maxime. Also: thank you very much for all your work. I haven't been doing a lot of Fable work in the last couple of months, so the current developer experience really blew me away. ❤

@WalternativE WalternativE deleted the consume-body-only-when-needed branch November 26, 2019 15:06
@MangelMaxime
Copy link
Contributor

Thank you for telling me and really happy that you like the developer experience :)

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.

2 participants