Update fetcher.ts to work like the browser

[menduz]

This PR removes a forceful throw and return undefined to copy the default behavior of the browser.

---

Context

I'm trying to use

if(request.status == 400) { 
  ...
}

but the

if !request.ok then throw

was preventing me from doing it

[menduz]

@juanmahidalgo @aleortega if you are OK with the approach, I'll correct the tests.

With retries, I'd return the last try's response

[juanmahidalgo]

Suggested change

	if (!isUsingNode() && !!response) {
	if (!isUsingNode() && response?.ok) {

do we need to add response?.ok here so it creates the buffer just if response.ok is truthy?

[menduz]

I think the buffer should always be available. 400 and 500 errors also have response bodies

[juanmahidalgo]

yeah, true, they should return the buffer as well. LGTM then!!

[aleortega]

@juanmahidalgo @aleortega if you are OK with the approach, I'll correct the tests.

With retries, I'd return the last try's response

Hello, looks good to me as well, in fact this was my first approach back then but received a feedback from the team saying that they preferred to throw in case of StatusCode !== 2xx. Not sure if it is okay adding an option to force throws or in this case (to avoid breaking changes), prevent throwing if a specific option is provided. What do you think?

Anyways, this is not a strong opinion but may this change will bring us the versatility to opt-in/out the preferred approach.

[menduz]

I think it is a ..weird behavior, I was trying to get the error message and fields from the 401 response; and instead I got a hard to parse exception message.

In the long term, I think the best is not to assume that this "throw" is a good thing and do server code as we do in the front-end, checking for the response.ok every time depending on the use case. That way anyone can leverage their knowledge about how "fetch" works, without any surprise bound to this implementation. Otherwise, I'd advice to name the function differently to "fetch" following the maxima of functional programming "if the function does, receives or returns something different, it should be named differently".

[aleortega]

Got it, so I'm ok with merging this. I'll update the code using this fetch to adapt it to the new implementation.

Thanks for the heads-up!

[hugoArregui]

@menduz are you going to work on the tests? I want to integrate this module but I would like to have this merged first, thanks!

[menduz]

I had to shift priorities, do you want to take it?

[aleortega]

@menduz are you going to work on the tests? I want to integrate this module but I would like to have this merged first, thanks!

I have already updated the tests weeks ago, but also had to shift priorities. Let me see if I can recover, adapt and push that commit today.

[aleortega]

Hey @menduz @hugoArregui,

I've just pushed the tests. I had already adapted these tests in the feat/optin-prevent-throwing branch, which I now regret creating haha. I believe this approach is clearer and it's better not to have an opt-in/opt-out mechanism for this.

After merging this PR I'm deleting the mentioned branch.

[aleortega]

[...] I had already adapted these tests in the feat/optin-prevent-throwing branch [...]

In case you're curious about the other approach, here is the PR.

[menduz]

thanks for the context, the tests and the experiment Ale! I also think it looks cleaner without the opt-in.

I think we are go for mering and releasing a new major. le damos?

[aleortega]

I think we are go for mering and releasing a new major. le damos?

Yass, all yours :) I'll let the team know about the change and start upgrading some services where I can use this (eager to remove those try/catches).