afterResponse hook doesn't work with the new expanded retry option

[sindresorhus]

Introduced by d4ddb7a.

This test now fails:

ky/test/hooks.js

Line 249 in a50948a

test.failing('`afterResponse` hook is called with input, normalized options, and response which can be used to retry', async t => {

 hooks › `afterResponse` hook is called with input, normalized options, and response which can be used to retry

  /Users/sindresorhus/dev/oss/ky/index.js:3

   2:
   3: const getGlobal = property => {
   4:   /* istanbul ignore next */

  Rejected promise returned by test. Reason:

  Error {
    message: 'retry.methods must be an array',
  }

  normalizeRetryOptions (index.js:3:1536)
  new Ky (index.js:4:7)
  ky (index.js:8:1713)
  t.deepEqual.ky.post.hooks.afterResponse (test/hooks.js:275:15)
  fn (index.js:6:55)

// @whitecrownclown @lambdalisue

[whitecrownclown]

The issue here is that the afterResponse hook uses the already normalized options for retry and methods (methods is a Set). An error is thrown since they go through the same normalize process again inside this test when calling ky.

@sindresorhus
What do you think about not throwing an error, but a promise rejection and also allow using both Array and Set as options? (so you can use afterResponse hook without altering the options)

[sholladay]

Some brainstorming and technical details here...

PR #180 fixed the failing test. It did so by filtering the options passed to hooks such that hooks only receive fetch/unknown options but no Ky options. In other words,retry and other Ky options are omitted from the options passed to hooks, so when the hook in that test gives its options back to Ky, no error is thrown, because retry is not there to be validated.

This works well enough and perhaps we should keep it that way. I've gone back and forth multiple times on whether I think hooks should receive Ky options. This issue, and the test being discussed here, made me think, "Yeah, hooks should receive all Ky options." So I went to go implement that and I changed the retry validation to allow Sets, but the test failed anyway because the normalized options have both json and body and Ky doesn't allow that (it considers those options to be mutually exclusive). This problem was never experienced before because prior to PR #180, some Ky options weren't given to hooks (including json), while others were. I'm not sure any intentional thought was put into it, the codebase just grew that way organically. So, in other words, solving this issue properly is not just a matter of how our validation works, it's also a matter of which Ky options are passed to hooks.

Ways forward from here:

1. Include all Ky options in the options object passed to hooks, for consistency/clarity. But then we need to make sure that those options validate successfully if we want to support the use case of a hook passing its options to Ky. For example, allow Sets in retry, and instead of validating that body and json are exclusive, simply let json take precedence over body. That way, no error will be thrown even if a hook does pass its options to Ky.
2. Include some (but not all) Ky options in the options object passed to hooks, like how it was before PR Use request to normalize options #180 (Ky v0.15 and earlier). Perhaps exclude json, retry, and any other options where the normalized form could cause validation errors, in case a hook passes its options to Ky. The hook can still parse the body manually, if it needs to.
3. Include no Ky options in the options object passed to hooks, like how it is on the master branch (which has not yet been released). It would still be possible for a hook to call Ky and have it "inherit" the Ky options, they would just need to use an extended Ky instance that is the same instance the hook itself was registered on.

As for how they compare, pros/cons:

• 1 has the advantage of being the most flexible for the user, but the downside is that if we want to support the use case in that test, then we need to loosen our validation (e.g. no error if both json and body are provided).
• 2 has the advantage of allowing us to keep our validation strict as it currently is, but the downside is that it's weird and confusing to have access to only some Ky options, plus the internals to make this work are a tiny bit more tedious.
• 3 has the advantage of allowing us to keep our validation strict as it currently is, and it's simpler conceptually, plus it's already implemented, but the downside is that hooks aren't able to inspect the value of options like prefixUrl, for example.

1 and 3 seem the most elegant to me overall. But the choices around validation have made the choice a little harder for me. I'm currently leaning towards 1 and I'm going to try that in an upcoming PR. Happy to hear any thoughts!