Skip to content

Fix extend() dropping numeric retry limit when merging with an object#867

Open
chatman-media wants to merge 2 commits into
sindresorhus:mainfrom
chatman-media:fix/extend-numeric-retry-limit
Open

Fix extend() dropping numeric retry limit when merging with an object#867
chatman-media wants to merge 2 commits into
sindresorhus:mainfrom
chatman-media:fix/extend-numeric-retry-limit

Conversation

@chatman-media

Copy link
Copy Markdown

Problem

retry accepts a number as shorthand for {limit: number} (per the docs: "If retry is a number, it will be used as limit and other defaults will remain in place.").

However, when a numeric retry is set on a parent instance and then extended with an object, the numeric limit is silently dropped and falls back to the default (2):

import ky from 'ky';

const api = ky.create({retry: 3});

// Intent: keep limit 3, only narrow the retriable methods.
const extended = api.extend({retry: {methods: ['get']}});

// Bug: `extended` retries with the default limit of 2, not 3.

The reverse direction (object on the parent, number on the child) and the all-object case both work correctly — only the number → object merge loses data.

Cause

In deepMerge, retry is merged like any other nested object. When the parent value is the numeric shorthand (3) and the incoming value is an object, the recursion ends up as deepMerge(3, {methods: ['get']}). A non-object source is skipped entirely by deepMerge, so the 3 is discarded and only {methods: ['get']} survives — the limit is lost.

Fix

Expand the numeric retry shorthand to {limit: number} before the deep merge, so extending it with an object preserves the limit. This mirrors the documented shorthand semantics and leaves every other case (object → object, object → number replacement, replaceOption) unchanged.

deepMerge({retry: 3}, {retry: {methods: ['get']}})
// before: {retry: {methods: ['get']}}
// after:  {retry: {limit: 3, methods: ['get']}}

Test

Added a test in test/retry.ts that sets retry: 3 on a parent, extends it with retry: {methods: ['get']}, and asserts the request is attempted limit + 1 times. It fails on main (3 attempts) and passes with the fix (4 attempts).

@sindresorhus

Copy link
Copy Markdown
Owner

I think this needs a small tweak. The new retry shorthand handling runs inside the generic recursive merge, so it can also affect user data, not just the top-level option.

For example, this now rewrites the JSON payload:

ky.create({
	method: 'post',
	json: {retry: 3},
}).extend({
	json: {retry: {foo: 'bar'}},
});

The request body becomes:

{"retry":{"limit":3,"foo":"bar"}}

I would expect it to stay:

{"retry":{"foo":"bar"}}

So the special-case probably needs to be scoped to the root options merge. I manually verified the current behavior with a custom fetch that reads the outgoing POST body, so this is not just a helper-level concern. Also worth adding this as a regression test so we don't accidently rewrite arbitrary payload data with a retry key.

@chatman-media

Copy link
Copy Markdown
Author

Good catch — fixed in 61b90ed. The shorthand expansion is now scoped to the root options merge via an isRoot flag, so nested user data containing a retry key (e.g. a json body) is no longer rewritten. The recursive merge calls pass isRoot: false, so only the top-level retry option gets the number→{limit} treatment.

Added your exact example as a regression test (asserting the outgoing POST body via the test server): it fails before the change ({retry: {limit: 3, foo: 'bar'}}) and passes after ({retry: {foo: 'bar'}}). All 86 retry tests green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants