Add client option to pass custom RequestInit object into fetch requests for supported implementations

[mellster2012]

Changes

What does this PR change? Link to any related issue(s).

#1837
#1631

How to Review

How can a reviewer review your changes? What should be kept in mind for this review?

Checklist

• Unit tests updated
• docs/ updated (if necessary)
• pnpm run update:examples run (only applicable for openapi-typescript)

[changeset-bot]

🦋 Changeset detected

Latest commit: f5a3b41

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages

Name	Type
openapi-fetch	Patch
openapi-react-query	Patch
swr-openapi	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[mellster2012]

          > My suggestion would be to revert back to providing the optional addition of custom RequestInit properties into the ClientOptions, e.g. RequestInitExtensions of type Record<string, unknown>, so any number of custom properties can be added, and pass them to the fetch call as 2nd parameter as RequestInit when present (like it's currently proposed in this PR), with the user assuming the control and responsibility of using the correct custom fetch implementation with it.

@drwpow @kerwanp let me know what you think - thanks!

I’m on board with trying that. Currently the problem with using the 2nd parameter approach is we found out that the fetch() implementation is different across different browsers and Node 😓. Specifically, most implementations want EITHER ✅ fetch(Request) OR ✅ fetch(string, RequestInit object), but NEVER ❌ fetch(string, Request) or ❌ fetch(Request, object).

Fortunately we added tests to catch this that does catch some gamebreaking regressions from happening again, but taking a 2nd look over everything I’m not confident we have all the tests in place (will work on adding that). All that said, for now, we’re somewhat stuck with the single-param fetch API of fetch(Request) as long as we have our current middleware API that uses Request/Response for performance and minimalism. But to that end, we can adjust the middleware API too and improve that with, say, a proper RequestInit object. But one decision affects the other, and vice-versa.

Sorry for the extra context; this just overlaps several other APIs/behavior, and past regressions, so I wanted to spend a little more thought on it.

Also the fetch implementation here by default does not provide an optional RequestInit, rather an expanded requestInit is hidden within the CustomRequest type, which seems already like a deviation.

To previous point, the CustomRequest type extends the built-in Request class, and though it does allow for additional properties, is still a valid implementation of the fetch spec that works in Node.js and all browsers, and is tested and working. So any changes to the fetch() call will just need a LOT of testing.

Originally posted by @drwpow in #1837 (comment)

Thanks for the context. Correct me if I'm wrong, in the JS runtime calling fetch(Request, undefined) is essentially the same as calling fetch(Request) - so it should never break anything. This PR adds a check trying to do its best to determine we are running in a recent enough node version and otherwise resets the RequestInit object to undefined, assuming running within node.js account for 99% of use cases for this option. This check can be extended via contributions to accept other implementations as well.

[mellster2012]

I noticed the tests have all been switched to passing in mocked responses, so the added real-world test may not pass muster. Any guidance on next steps to bring this closer to completion (if this feature is still desired) would be helpful - thanks!

[drwpow]

Thanks for the context. Correct me if I'm wrong, in the JS runtime calling fetch(Request, undefined) is essentially the same as calling fetch(Request) - so it should never break anything. This PR adds a check trying to do its best to determine we are running in a recent enough node version and otherwise resets the RequestInit object to undefined, assuming running within node.js account for 99% of use cases for this option. This check can be extended via contributions to accept other implementations as well.

You have a very good point! I think you’re right. Thanks for taking the time to talk this over. Again, if we hadn’t shipped this bug in a previous version of openapi-fetch, I wouldn’t sweat it as much.

I noticed the tests have all been switched to passing in mocked responses, so the added real-world test may not pass muster.

Hm yeah we do have a .e2e.ts test that runs in Playwright for browsers, just to make sure the core fetch() call works according to all the different implementations (for the reasons we just discussed). But we don’t currently have an unmocked version of the same in Node.js. I don’t think that’s a hard requirement for this PR, but if you wanted to make a single, standalone test that talked to an Express server, you could, e.g.

import express from 'express'
import { beforeAll, describe, expect, test } from 'vitest'

const API_PORT = process.env.API_PORT || 4578

const app = express();
app.get('/v1/my-endpoint', (req, res) => {
  res.send(foo);
});

test('my test', async () => {
  const client = createClient({ baseUrl: `http://localhost:${API_PORT}` });

  // rest of test
});

That way it’s making actual fetch() requests in Node; we’re just sending it to localhost. In all honesty, the rest of our tests should probably do this too 🤔. I don’t have a good reason why we didn’t (I just didn’t think of it)

[drwpow]

As an aside, we were using Mock Service Worker for the longest time, but that was overkill, slowed tests down, and when we wanted to run performance benchmarks, MSW would cause them to flake. So we removed it. Again, not a requirement for this PR but after talking, I would like to improve our testing setup to test the surface area of native Node.js fetch (especially now that it’s 100% supported by all LTS versions)

[mellster2012]

Thanks for the context. Correct me if I'm wrong, in the JS runtime calling fetch(Request, undefined) is essentially the same as calling fetch(Request) - so it should never break anything. This PR adds a check trying to do its best to determine we are running in a recent enough node version and otherwise resets the RequestInit object to undefined, assuming running within node.js account for 99% of use cases for this option. This check can be extended via contributions to accept other implementations as well.

You have a very good point! I think you’re right. Thanks for taking the time to talk this over. Again, if we hadn’t shipped this bug in a previous version of openapi-fetch, I wouldn’t sweat it as much.

I noticed the tests have all been switched to passing in mocked responses, so the added real-world test may not pass muster.

Hm yeah we do have a .e2e.ts test that runs in Playwright for browsers, just to make sure the core fetch() call works according to all the different implementations (for the reasons we just discussed). But we don’t currently have an unmocked version of the same in Node.js. I don’t think that’s a hard requirement for this PR, but if you wanted to make a single, standalone test that talked to an Express server, you could, e.g.

import express from 'express'
import { beforeAll, describe, expect, test } from 'vitest'

const API_PORT = process.env.API_PORT || 4578

const app = express();
app.get('/v1/my-endpoint', (req, res) => {
  res.send(foo);
});

test('my test', async () => {
  const client = createClient({ baseUrl: `http://localhost:${API_PORT}` });

  // rest of test
});

That way it’s making actual fetch() requests in Node; we’re just sending it to localhost. In all honesty, the rest of our tests should probably do this too 🤔. I don’t have a good reason why we didn’t (I just didn’t think of it)

Done (thanks for the pointers!) - do we need a changeset and/or docs update or are we good?

[drwpow]

Done (thanks for the pointers!) - do we need a changeset and/or docs update or are we good?

Yes a patch changeset would be great, just because this is an improvement that should be called out in the changelog! Also sorry for the minor conflict, but once a changeset is in and conflict resolved, we can merge & ship.

[mellster2012]

Done (thanks for the pointers!) - do we need a changeset and/or docs update or are we good?

Yes a patch changeset would be great, just because this is an improvement that should be called out in the changelog! Also sorry for the minor conflict, but once a changeset is in and conflict resolved, we can merge & ship.

Done. It's now complaining about a missing github token when building, I'm assuming this is to be fixed on the CI side, not related to this PR. Thanks!

[drwpow]

Done. It's now complaining about a missing github token when building, I'm assuming this is to be fixed on the CI side, not related to this PR. Thanks!

Oh shoot yes that was a change I made last night that needs to be fixed. Yes ignore that; that’s unrelated to this PR.