Allow extend() to add 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?

[sholladay]

... but this results in requests failing with errors like Uncaught TypeError: this._options.hooks.beforeRequest is not iterable

Sounds like it just needs to be coerced to an empty array somewhere.

Do we just need another specialized merge function?

Ugh, I hope not. 😅 As a maintainer, I find this part of the code hard to reason about. mergeHeaders on its own isn't too bad, just a little ugly with how it has to handle both header options and header instances, plus the string "undefined" because of the way the Headers class works. But when I have to think about how deepMerge uses it, and the recursive behavior of deepMerge, and all the different cases and iteration going on, it gets to be a bit much. It's not even fully typed because of the complexity...

As a user, it's convenient until it isn't, since the behavior is so magical.

I would like to see us move towards an API where ky.extend(foo) does a simple, shallow object spread of foo into the ky options. For more complex scenarios, you would use ky.extend(options => foo), which would still do the same object spread but by providing the existing options, it would allow you to use your own merge logic, which in most cases would be simple.

So, for example, to achieve the expected result from the original issue:

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 }) => {
  return {
    hooks: {
        ...hooks,
        beforeRequest: [...hooks.beforeRequest, requestToSnakeCase],
        afterResponse: [responseToCamelCase],
    }
  };
});

And if you wanted to delete a hook type, just set it to undefined (or don't spread it in):

import ky from 'ky';

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

// Ky which doesn't run any hooks for retries
export const kyWithoutRetryHooks = rootKy.extend(({ hooks }) => {
  return {
    hooks: {
        ...hooks,
        beforeRetry: undefined,
    }
  };
});

And now you can even delete a single, specific hook:

import ky from 'ky';

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

// Ky which doesn't log errors for retries
export const kyWithoutRetryLogs = rootKy.extend(({ hooks }) => {
  return {
    hooks: {
        ...hooks,
        beforeRetry: hooks.beforeRetry.filter(hook => hook !== logError),
    }
  };
});

And if a deep merge is truly necessary, it can still be done, just with less magic:

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((options) => {
  return deepMerge(options, {
    hooks: {
        beforeRequest: [requestToSnakeCase],
        afterResponse: [responseToCamelCase],
    }
  });
});

Is it a little more verbose in some cases? Sure. But it's very flexible, makes deleting easy, and makes it obvious what's going on. Ky could export some merge utilities for convenience. But the main code would only have to be concerned with normalizing, not merging.

This approach would solve various issues:

• Allow extend() to append to prefixUrl #586
• Setting searchParams to URLSearchParams both in create() and get() doesn't merge them correctly #491

[Kenneth-Sills]

@sholladay Hm... I agree, this is actually what I've done in a wrapper library already. But OTOH that would be a breaking change that directly inverts the behavior of an existing method, and I'd argue it isn't what many people would want behavior-wise (of course, maintainers are ultimately the deciding force in any project).

What would you say about continuing with the existing BC feature PR to update the behavior of deepMerge. Then I can prepare another PR for a separate .recreate()¹ method with the behavior you're describing, signaling preference for the latter in documentation. I'd also export deepMerge to the public API to satisfy your last example... maybe renamed as smartMerge()¹ since it's behavior deviates slightly from an actual deep merge?

It still brings a maintenance cost, admittedly, but that way we can get everyone an improved API without a breaking change while leaving the existing behavior for those who prefer it. Probably wouldn't be as concerned if it weren't a project with 1M+ weekly downloads. 😅

Footnotes

1. Still workshopping the name, feel free to throw in a suggestion. ↩ ↩²

[sholladay]

With PR #611 merged, you can now pass a function to ky.extend()! It still uses deepMerge(), not a shallow spread, but IMO it's a big step in the right direction. In principle, if it weren't for deepMerge() being used, you could now use any add/remove/sort behavior you want.

What would you say about continuing with the existing BC feature PR to update the behavior of deepMerge.

@Kenneth-Sills that all sounds good to me. 👍

I'm going to close this, as the thread is getting a bit long/old and the OP's issue was fixed some time ago. Feel free open a new issue for changes to deepMerge() and the ability to remove hooks.