fix: add verification when request body is falsy

[eduardohilariodev]

Changes

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

How to Review

The instanceof was throwing an error when the body was falsy.

Checklist

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

[drwpow]

Good fix! I’m fine merging this without tests as it’s simple enough, but would also love a test that prevents this regressing if possible.

[eduardohilariodev]

Good fix! I’m fine merging this without tests as it’s simple enough, but would also love a test that prevents this regressing if possible.

I am getting this error when running pnpm test

pnpm test

> [email protected] pretest /home/eduardo/dev/forks/openapi-typescript/packages/openapi-fetch
> pnpm run generate-types


> [email protected] generate-types /home/eduardo/dev/forks/openapi-typescript/packages/openapi-fetch
> node ./scripts/generate-types.js

file:///home/eduardo/dev/forks/openapi-typescript/node_modules/.pnpm/[email protected]/node_modules/execa/lib/error.js:60
                error = new Error(message);
                        ^

Error: Command failed with exit code 1: ../openapi-typescript/bin/cli.js ./test/fixtures/api.yaml -o ./test/fixtures/v7-beta.d.ts
node:internal/modules/esm/resolve:264
    throw new ERR_MODULE_NOT_FOUND(
          ^

Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/dist/index.js' imported from /home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/bin/cli.js
    at finalizeResolution (node:internal/modules/esm/resolve:264:11)
    at moduleResolve (node:internal/modules/esm/resolve:917:10)
    at defaultResolve (node:internal/modules/esm/resolve:1130:11)
    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)
    at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)
    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)
    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)
    at link (node:internal/modules/esm/module_job:84:36) {
  code: 'ERR_MODULE_NOT_FOUND',
  url: 'file:///home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/dist/index.js'
}

Node.js v20.11.1
    at makeError (file:///home/eduardo/dev/forks/openapi-typescript/node_modules/.pnpm/[email protected]/node_modules/execa/lib/error.js:60:11)
    at handlePromise (file:///home/eduardo/dev/forks/openapi-typescript/node_modules/.pnpm/[email protected]/node_modules/execa/index.js:124:26)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async Promise.all (index 1)
    at async generate (file:///home/eduardo/dev/forks/openapi-typescript/packages/openapi-fetch/scripts/generate-types.js:13:3) {
  shortMessage: 'Command failed with exit code 1: ../openapi-typescript/bin/cli.js ./test/fixtures/api.yaml -o ./test/fixtures/v7-beta.d.ts',
  command: '../openapi-typescript/bin/cli.js ./test/fixtures/api.yaml -o ./test/fixtures/v7-beta.d.ts',
  escapedCommand: '"../openapi-typescript/bin/cli.js" "./test/fixtures/api.yaml" -o "./test/fixtures/v7-beta.d.ts"',
  exitCode: 1,
  signal: undefined,
  signalDescription: undefined,
  stdout: '',
  stderr: 'node:internal/modules/esm/resolve:264\n' +
    '    throw new ERR_MODULE_NOT_FOUND(\n' +
    '          ^\n' +
    '\n' +
    "Error [ERR_MODULE_NOT_FOUND]: Cannot find module '/home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/dist/index.js' imported from /home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/bin/cli.js\n" +
    '    at finalizeResolution (node:internal/modules/esm/resolve:264:11)\n' +
    '    at moduleResolve (node:internal/modules/esm/resolve:917:10)\n' +
    '    at defaultResolve (node:internal/modules/esm/resolve:1130:11)\n' +
    '    at ModuleLoader.defaultResolve (node:internal/modules/esm/loader:396:12)\n' +
    '    at ModuleLoader.resolve (node:internal/modules/esm/loader:365:25)\n' +
    '    at ModuleLoader.getModuleJob (node:internal/modules/esm/loader:240:38)\n' +
    '    at ModuleWrap.<anonymous> (node:internal/modules/esm/module_job:85:39)\n' +
    '    at link (node:internal/modules/esm/module_job:84:36) {\n' +
    "  code: 'ERR_MODULE_NOT_FOUND',\n" +
    "  url: 'file:///home/eduardo/dev/forks/openapi-typescript/packages/openapi-typescript/dist/index.js'\n" +
    '}\n' +
    '\n' +
    'Node.js v20.11.1',
  cwd: '/home/eduardo/dev/forks/openapi-typescript/packages/openapi-fetch/',
  failed: true,
  timedOut: false,
  isCanceled: false,
  killed: false
}

Node.js v20.11.1
 ELIFECYCLE  Command failed with exit code 1.
 ELIFECYCLE  Command failed with exit code 1.
 ```

[drwpow]

You’ll need to run pnpm run build first (either in the root, or in packages/openapi-typescript; doesn’t matter).

The Contributing guide has some additional information on testing locally.

[eduardohilariodev]

I am having problems in understanding the trigger for the removal of this header. I am not familiar with the inner workings of this lib.

I made the start of a test, but I don't think it's quite it:

   it("deletes Content-Type header when body is FormData", async () => {
      const client = createClient<paths>({ baseUrl });

      const formData = new FormData();
      formData.append("key", "value");

      const { getRequest } = useMockRequestHandler({
        baseUrl,
        method: "post",
        path: "/anyMethod",
        status: 200,
        body: { success: true },
      });

      await client.POST("/anyMethod", {
        headers: {
          "Content-Type": "application/json", 
        },
      });

      const req = getRequest();

      expect(req).toBeDefined();
      expect(req.headers).toBeDefined();
      expect(req.headers.has("Content-Type")).toBe(false);
      expect(req.body).toBe(formData);
    });

[drwpow]

I am having problems in understanding the trigger for the removal of this header. I am not familiar with the inner workings of this lib.

At a high level, there are 2 ways of interpreting #1694:

1. We should unset "content-type" for all GET requests
2. We should respect the user’s preference if they set "content-type", but not send that header by default for GET requests.

This is opinionated, but the first option seems overreaching, and having read a lot of bug reports and strange ways APIs behave, I feel confident that there will likely be some users that have APIs that require "content-type" to be set on GET requests (regardless of what any specification says; there are lots of API frameworks that have the weirdest requirements and I hear “we wish we could but we can’t change it” often). And we’d have to change it to the second option anyway when that first bug report comes in. The second option feels much safer, as it ultimately lets the user decide, but also provides good defaults. I would strongly suggest the second approach.

For #1695, what we want to accomplish is only set "content-type": "application/json" if body is not undefined (if (requestInit.body !== undefined)).

Solving the 2 together, on this line where we initialize the headers, we should just remove that. Unsetting a header is tricky because we don’t know if we set it or the user. So we can just delay setting that in the first place until the actual request (meaning it will be undefined by default).

-  baseHeaders = mergeHeaders(DEFAULT_HEADERS, baseHeaders);

Later, when we create the request object, we are again merging headers (which had the default "content-type": "application/json" header unless the user overrode it). Right before that happens, we want to add something like:

+     if (requestInit.body !== `undefined`) {
+       baseHeaders = mergeHeaders(DEFAULT_HEADERS, baseHeaders);
+     }
+ 
      const requestInit = {

Which we couldn’t do earlier, because we didn’t know if there was a body on the request or not. Also, making sure DEFAULT_HEADERS is first in the list is important, because any conflict will make sure it isn’t set.

So what we’ve done is just delay the default until later, and IF AND ONLY IF this has a body.

---

I made the start of a test, but I don't think it's quite it:

The existing logic is there because we want to have "content-type": "multipart/form-data" when FormData is passed for the body. That’s unrelated to your fix. Your test(s) needs to cover:

1. For all GET requests (i.e. requests that don’t have a body), "content-type": is undefined by default (when the user never specified)
2. For GET requests, "content-type" keeps options passed into createClient() if the user manually specified one
3. For GET requests, "content-type" keeps any value passed in on individual requests (overrides)
4. For (any non-GET; doesn’t matter which) requests with a body, "content-type" is "application/json" by default

With the danger here being we accidentally invert the order, and unset "content-type" if the user has manually declared it.

Remember that the approach would differ if we had chosen the first option in the beginning—we’d simply delete that header regardless of what the user did. But as that feels more dangerous/unpredictable, we structured our code and the tests to make sure we don’t override users’ opinions, and instead just improve the defaults.

[imranbarbhuiya]

Suggested change

	if (requestInit.body && requestInit.body instanceof FormData) {
	if (!requestInit.body || requestInit.body instanceof FormData) {

altho out of scope for this pr but we should delete the content type when body is empty, currently there's no proper way to remove the body and when I don't need to send any body to backend, backend throws content type json was used but no body provided

Edit: ignore this, for some reason github didn't loaded the above conversations which has discussions related to it

[drwpow]

Yeah I may have approved prematurely because I didn‘t realize at first this was attempting to solve 2 issues with 1 PR. My suggestions should be a ~2-line change in code, and a couple test additions to satisfy both.

[changeset-bot]

⚠️ No Changeset found

Latest commit: 429b2b0

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

[eduardohilariodev]

@drwpow Do you think it's necessary to perform these tests on this specific PR? I am encountering this bug every time I reinstall this lib, having to change it locally to be able to test my app (with Vitest). Do you think your suggestions can be put into another issue that has a more comprehensive context? The way I see them, they alter the library's behaviour (much more overarching than this simple hotfix). Please, do correct me if I am ignorant about this.

[drwpow]

@drwpow Do you think it's necessary to perform these tests on this specific PR? I am encountering this bug every time I reinstall this lib, having to change it locally to be able to test my app (with Vitest). Do you think your suggestions can be put into another issue that has a more comprehensive context? The way I see them, they alter the library's behaviour (much more overarching than this simple hotfix). Please, do correct me if I am ignorant about this.

Yes we can merge as-is, if you’d like. But I mostly asked for tests because in order to fix all the problems outlined in #1694 and #1965, we’d probably have to overwrite your changes here (which would be a slight behavior change, as you pointed out, but IMO nothing significant or breaking). I believe your original issue overlaps a little with those issues, but is actually different. Tests are just good reinforcers that when you write code, it sticks around (because you taking your valuable time to open a PR and fix an issue means a lot! It really does. And this project is only possible from people like yourself. And tests just ensure your good work sticks around).

Given the time on this, and the need to fix #1694 and #1695, I’ll merge, and push a fix for those separately. But feel free to open another issue (or another PR) if those changes cause a regression for your scenario.