Allow extend() to add/remove hooks

[ma101an]

Two Code Snippets

import ky from 'ky';

// Root Ky
const rootKy = ky.create({
  prefixUrl: '/api',
  timeout: false,
  
  hooks: {
    beforeRequest: [pruneRequestPayload],
    beforeRetry: [logError],
  },
});

// Ky which serializes request and response
export const kyWithSerializer = rootKy.extend({
  hooks: {
    beforeRequest: [requestToSnakeCase],
    afterResponse: [responseToCamelCase],
  },
});

Expectation: kyWithSerializer to also execute pruneRequestPayload in beforeRequest hook and should have beforeRetry present too Actual: beforeRequest hook array is completely replaced with new array

[sindresorhus]

The problem is that both scenarios are valid. A user might want to replace existing hooks and they might want to accumulate. If we support what you propose, there will be no way to replace existing hooks.

I'm open to suggestions on how to solve that.

Either way, we should more clearly document the behavior.

[sindresorhus]

I do agree it makes sense to merge the hooks arrays by default.

Maybe we could do something like this to support replacing:

import ky, {replaceHooks} from 'ky';

const rootKy = ky.create({
  prefixUrl: '/api',
  timeout: false,

  hooks: {
    beforeRequest: [pruneRequestPayload],
    beforeRetry: [logError],
  },
});

export const kyWithSerializer = rootKy.extend({
  hooks: {
    beforeRequest: replaceHooks([requestToSnakeCase]),
    afterResponse: [responseToCamelCase],
  },
});

Where replaceHooks just adds a special Symbol that we could check during the merge process.

[ma101an]

@sindresorhus, yeah I like this idea. it's granular to handle each hooks separately.

[sholladay]

Both scenarios are definitely valid use cases, and I can understand the argument that .extend() should add to the existing hooks by default, not replace them. But I strongly dislike having some kind of replaceHooks API. This problem really isn't specific to hooks, it's not even specific to Ky. It's something I encounter in many APIs that accept multiple values. For example, hapi.js has a cors setting which lets you control how the server responds to CORS requests and it has a default list of response headers, which you may want to either replace or add to. They let you make that decision by having two options: headers replaces the list and additionalHeaders adds to the list.

I suppose in Ky we could have an equivalent additionalHooks option.

Another approach would be to allow the user to pass in a function which is given the default / existing list and returns the new list after any replace / accumulate logic. This is essentially inversion of control.

import ky from 'ky';

const rootKy = ky.create({
  prefixUrl: '/api',
  timeout: false,

  hooks: {
    beforeRequest: [pruneRequestPayload],
    beforeRetry: [logError],
  },
});

export const kyWithSerializer = rootKy.extend({
  hooks: {
    // spread the existing hooks to keep them (accumulate) or omit them to overwrite (replace)
    beforeRequest: (beforeRequestHooks) => [...beforeRequestHooks, requestToSnakeCase],
    afterResponse: [responseToCamelCase],
  },
});

[ma101an]

@sholladay I agree, I did not find the replaceHooks method very "elegant" actually.

But I've also thought about "passing a callback function" technique. One thing which concerns me is what about the hooks that are not mentioned at all. for example, beforeRetry is not mentioned when extending but it's supposed to be present ideally. which would lead us to write something like

hooks: {
  beforeRetry: (beforeRetry) => [...beforeRetry]
}

PS: I like the additionalHooks approach better than replaceHooks, just have no idea how much complicated it would be to add it to the ky.extend options

[sholladay]

IMO, the .extend() method should probably use object spread on hooks. That's all that's needed to fix the problem of beforeRetry getting unexpectedly deleted when extending. The user could still do beforeRetry: undefined if they intend to delete it.

[ma101an]

@sholladay @sindresorhus To Summarize:

• Add support for providing callbacks for any hook to solve extending hooks, or providing completely new ones.
• Use Object Spread on hooks prop alone to avoid hooks getting unexpectedly deleted when extending

[sindresorhus]

IMO, the .extend() method should probably use object spread on hooks. That's all that's needed to fix the problem of beforeRetry getting unexpectedly deleted when extending. The user could still do beforeRetry: undefined if they intend to delete it.

👍

[sindresorhus]

But I strongly dislike having some kind of replaceHooks API.

I'm curious why you don't like it? As I find it quite elegant. Definitely more elegant than additionalHooks.

Although, accepting a function would work too, and it has the added benefit of allowing more merging cases.

[agierlicki]

What do you guys think about a replaceHooks option on the object that is passed to ky.extend? If true the hooks will replace the old ones and if false (default) they will get appended.

[sindresorhus]

@agierlicki I think it needs to be more fine-grained than that.

[blockmood]

What about follow up?

[Kenneth-Sills]

I'm not sure when this changed (the move to Typescript obfuscated the blame history a little), but the library now works the way the ticket author requested:

const client1 = ky.create({ hooks: {
    beforeRequest: [ () => console.log('before 1') ],
    afterResponse: [ () => console.log('after 1') ],
}});

const client2 = client1.extend({ hooks: {
    beforeRequest: [ () => console.log('before 2') ],
    afterResponse: [ () => console.log('after 2') ],
}});

await client1.get('https://www.google.com');
    // before 1
    // after 1
await client2.get('https://www.google.com');
    // before 1
    // before 2
    // after 1
    // after 2

Unfortunately, as pointed out earlier, this does mean there's no way to remove these hooks. My intuition said to try doing as headers do (also hinted to by Seth above) and explicitly specifying undefined for the hooks I wanted removed (this does technically pass type checking), but this results in requests failing with errors like Uncaught TypeError: this._options.hooks.beforeRequest is not iterable.

Do we just need another specialized merge function?