Throw error when ky.stop is returned

[jabuj]

Fixes #290. This also may be a breaking change although no one seems to have opened an issue that returning ky.stop causes TypeError until now.

[szmarczak]

Looks good. Maybe we need to add a line to make sure that the error contains 400 status code.

[sholladay]

Please add assertions about the thrown error. Otherwise, for all we know Ky is experiencing some kind of internal error.

[sholladay]

This change makes sense to me, in so far as I believe that throwing is probably the behavior people generally want when stopping retries. But looking at it more, it makes me question the value of ky.stop. I mean, beforeRetry hooks already receive the error as an argument and users can just throw error themselves.

ky.get('http://example.com', {
    hooks: {
        beforeRetry: [
            ({error}) => {
                if (someCondition) {
                    throw error;  // <-- stops retries and short-circuits beforeRetry loop
                }
            }
        ]
    }
});		

The above has always been possible, actually. But there's no way for users to short-circuit the beforeRetry loop without throwing unless Ky takes care of that internally (i.e. the existing behavior before this PR).

So, perhaps this is really a documentation issue and we should encourage users to throw error themselves and keep ky.stop as it is, for the niche case where you want to stop retries but have Ky "succeed"? Or, is there a good reason to have some sugar for throw error?

[szmarczak]

So, perhaps this is really a documentation issue and we should encourage users to throw error themselves and keep ky.stop as it is, for the niche case where you want to stop retries but have Ky "succeed"?

I agree with @sholladay

[jabuj]

I don't quite agree with such solution.

ky.get('https://example.com', { beforeRetry: [() => ky.stop] })
  .json()
  .then(data => console.log(data.property))

So do you suggest that in this simple case, if retry is triggered and beforeRetry returns ky.stop, we just tweak text() and json() and whatever methods so that they don't throw? In this case what is data supposed to be, undefined? This would basically kill one of the selling points of ky:

Treats non-2xx status codes as errors (after redirects)

This behavior is preferable in the first place because it lets users replace ifs that check if request was successful to more logical try/catch blocks or .catch methods if they are working with promises and generally get all advantages of errors. But now it turns out that data might be undefined, so one should not only handle http and network errors but also check that data is not undefined which is not nice.

So yes, there are situations when fetch doesn't return the body of the response, but someone prefers to ignore these cases and treat these as unexpected errors that are handled on some more global level of application and not to check that every received request is not undefined. And if we fix this without throwing an error, this can potentially lead to many places where such checks will be needed, because those may happen way more often than fetch returning nothing

Considering this, I think it would be better either to remove ky.stop completely or leave it as syntax sugar for throwing original error. I might've missed some other solutions or cases though so not sure about that

[sholladay]

I don't quite agree with such solution.

I'm not sure what you mean. Are you saying that you want to remove ky.stop from the API?

So do you suggest that in this simple case, if retry is triggered and beforeRetry returns ky.stop, we just tweak text() and json() and whatever methods so that they don't throw? In this case what is data supposed to be, undefined?

Well, I think it's important to keep in mind that not everyone is using body method shortcuts (e.g. ky().json()). For people who are not using those methods, with the existing versions of Ky and ky.stop, there won't be any problem. The response is undefined but there is no parsing error since they aren't trying to parse the body of the response.

For people who are using body method shortcuts, like in your example, an error will be thrown because there's no body to parse (in fact, even before then, Ky actually runs into a TypeError when we try to clone() an undefined response). You're right that we could override json() (et al) to return empty data structures instead of throwing. I guess I would be fine with that. Although, it seems a bit unnecessarily complicated. I think it's good enough to just leave a note in the docs that says ky.stop is incompatible with body method shortcuts, and recommend using throw error instead.

[szmarczak]

You're right that we could override json() (et al) to return empty data structures instead of throwing. I guess I would be fine with that.

I agree too.

[jabuj]

For people who are not using those methods, with the existing versions of Ky and ky.stop, there won't be any problem.

Well, it actually seems that we need to make some tweaks for those as well. Like here:

ky.put("https://httpstat.us/500/cors", {
  hooks: { beforeRetry: [() => ky.stop] }
})
  .then((res) => console.log(res.ok))

If ky.stop is returned, res will be undefined which certainly doesn't make sense. I don't think anyone would agree there is an adequate reason for the response object to be undefined. This may happen if there was a fetch error but in that case TypeError is thrown so no checks for res !== undefined needed. Not with ky.stop though

I'm not sure what you mean. Are you saying that you want to remove ky.stop from the API?

Now that would probably be silly, since someone may be using it, so another option I wrote at the end of my comment: leave it as syntactic sugar for throw error. Your suggestion to just stating that ky.stop is incompatible with shortcut methods seems reasonable. Yet after thinking about it I still don't quite agree. That just doesn't seem intuitive to me, like I personally don't expect response data to be undefined under any circumstances other than a weird fetch API quirk that happens once an eternity. Yes, this won't happen if I throw error instead of return ky.stop. But this thing is still mentioned in docs so it may be a moment of pain for someone who sees it there and decides to use it. Actively discouraging its use is a solution, but then what is the reason to leave it in docs in the first place, may be it's better to just never mention it and leave for backwards compatibility?

Also leaving return ky.stop as syntactic sugar for throw error looks like a nice option to me. Throwing that error manually looks a bit like I have to handle some internal functionality that a library should be doing for me. I mean, returning ky.stop in a way increases level of abstraction which in this case IMHO looks nice. But this one is way too subjective to be considered a strong argument. It would be nice to hear more feedback about desired behaviour actually

[sholladay]

Thanks for clarifying. Your points make sense to me but my stance is still that ky.stop is extra functionality you don't have to use, albeit perhaps of limited value in the end.

If ky.stop is returned, res will be undefined which certainly doesn't make sense.

I disagree. Whether it throws an error or not, the whole purpose of ky.stop is to say, "Hey, I give up on getting a response". When you use ky.stop, you are opting into this behavior. I get your point about how when Ky succeeds, your code should generally be able to assume there is a response, and I think you make a fair point. Having to check for a response is a bit inconvenient, but again, you only have to do that if you use this feature. Also, consider that you can use optional chaining, as well as nullish coalescing to simplify how you handle the undefined response.

For example, this would run successfully without a property access error because of the use of optional chaining:

ky.put("https://httpstat.us/500/cors", {
  hooks: { beforeRetry: [() => ky.stop] }
})
  .then((res) => console.log(res?.ok))

Another thing to consider is that I've personally written code where Ky is merely a fire-and-forget side effect and I never use the response. An example is when POSTing non-critical metrics data in an app. I can imagine use cases where you'd want to fire-and-forget with custom retry logic that uses ky.stop.

The way I'm thinking about this is that ky.stop is like the return (or break) keyword. You can use either return or throw to exit a function. It makes sense that you can return or break without a particular result, but I would hate it if you could throw without a particular error. If ky.stop is used to indicate a failure, then shouldn't we make it a function where you have to pass an error or error message? If the error is just a generic, "Error: ky.stop was used, that doesn't seem useful and doesn't seem like a reason to crash the program, either. To me, using ky.stopseems more likeprocess.exit(0)thanprocess.exit(1)` in Node.

In other words, when a user decides to give up on retrying, I think they ought to have options regarding whether the operation is considered successful or unsuccessful. Right now, ky.stop is the only way to indicate success. If we make it throw, there will be no way to do that. I suppose we could introduce some other mechanism to exit successfully, but what would we call it if not ky.stop?

[jabuj]

The way I'm thinking about this is that ky.stop is like the return (or break) keyword

Thinking about this way, it makes sense, thanks for clarification. Well, after thinking about this, I think I agree with this solution. But there must be a big warning in readme that says ky.stop is intended like return;. I see it as a legimiate use for side-effect requests but not for anything that is supposed to fetch actual data, so this should be clarified in readme. Better yet it would be nice if .json() and co. showed a warning in the console when ky.stop is used.

[sholladay]

For now, let's update the code examples to use throw error and add that caveat to ky.stop. I think changes to body methods should be done separately. If you're up for contributing those documentation updates, this PR or a new one is fine.

[sholladay]

Closing in favor of #314