Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined properties on body object are not flagged if required fields are filled in #1769

Open
1 task
RPGillespie6 opened this issue Jul 17, 2024 · 8 comments
Assignees
Labels
bug Something isn't working good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue!

Comments

@RPGillespie6
Copy link

RPGillespie6 commented Jul 17, 2024

Description

If I have something like this:

const { data, error } = await client.POST("/api/product", {
    body: {
        ProductVersion: "1.0.0",
        ASDfadsfasdf: "asdfasdf"
    }
});

then tsc correctly flags ASDfadsfasdf as being superfluous:

npx tsc --noEmit

test.ts:15:9 - error TS2353: Object literal may only specify known properties, and 'ASDfadsfasdf' does not exist in type '{ FileData: string; ProductVersion: string; }'.

15         ASDfadsfasdf: "asdfasdf"

However, as soon as all required fields are in:

const { data, error } = await client.POST("/api/product", {
    body: {
        ProductVersion: "1.0.0",
        FileData: "test",
        ASDfadsfasdf: "asdfasdf"
    }
});

it is no longer flagged:

npx tsc --noEmit

This is a problem because it means you can introduce typos on optional fields, and tsc won't catch it.

Reproduction

Seems to happen no matter what I try. tsconfig is set to esnext module with bundler resolution.

Expected result

Superfluous fields should be flagged regardless of if required ones are filled in or not.

Checklist

@RPGillespie6 RPGillespie6 added bug Something isn't working openapi-fetch Relevant to the openapi-fetch library labels Jul 17, 2024
@RPGillespie6 RPGillespie6 changed the title Superfluous fields are not flagged if required fields are filled in Extra properties on body object are not flagged if required fields are filled in Jul 17, 2024
@RPGillespie6 RPGillespie6 changed the title Extra properties on body object are not flagged if required fields are filled in Extra (undefined) properties on body object are not flagged if required fields are filled in Jul 17, 2024
@RPGillespie6 RPGillespie6 changed the title Extra (undefined) properties on body object are not flagged if required fields are filled in Undefined properties on body object are not flagged if required fields are filled in Jul 17, 2024
@RPGillespie6
Copy link
Author

RPGillespie6 commented Jul 24, 2024

Ok, I think I've figured out more clues as to what's causing this. It doesn't necessarily have to do with required properties. If multiple components have similar properties (such as id), the type checking only works if you've defined enough properties to uniquely identify the component.

So for example, this is not flagged nor does jump to definition work because id alone doesn't uniquely identify the component:
image

but this is correctly flagged, and jump to definition is working again, because status uniquely identifies the Order component:
image

Also note that results are the same in the above 2 cases when doing npx tsc --noEmit, so I know it's not just a quirk of VSCode

My guess is that this is some quirk of generics. The end result is that the type-checking is unreliable because you can now have silent failures in the cases where you haven't defined enough properties to uniquely identify the component.

@XProfessional1130

This comment was marked as spam.

@hayderjebur
Copy link

First option: Using object spread can help catch extra properties!

const baseBody = {
    ProductVersion: "1.0.0",
    FileData: "test"
};
// Add an extra property with a spread
const body = {
    ...baseBody,
    ASDfadsfasdf: "asdfasdf" // TypeScript will flag this
};
const { data, error } = await client.POST("/api/product", { body });

Second option: Applying explicit type annotations forces TypeScript to perform a strict check!

const body: { FileData: string; ProductVersion: string } = {
    ProductVersion: "1.0.0",
    FileData: "test",
    ASDfadsfasdf: "asdfasdf" // This will now be flagged
};
const { data, error } = await client.POST("/api/product", { body });

@RPGillespie6
Copy link
Author

Thanks, but it appears these workarounds still don't catch the issue. For example:

import createClient from "openapi-fetch";
import type { paths } from "./petstore"; // generated by openapi-typescript

const client = createClient<paths>();

const baseBody: { id: number, bogus: string } = {
    id: 0,
    bogus: "bogus",
};

const {
    data, // only present if 2XX response
    error, // only present if 4XX or 5XX response
} = await client.POST("/store/order", {
    params: {},
    body: baseBody,
});

The bogus field does not get flagged by tsc

@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 12, 2024

@Elgueromiranda That defeats the purpose of openapi-typescript if I need to re-define the body types.

@iamnoah
Copy link

iamnoah commented Aug 13, 2024

Bitten by this today. I had a typo in an optional body parameter name that went completely unchecked.

@drwpow
Copy link
Contributor

drwpow commented Aug 14, 2024

This is a great catch, and would welcome a PR for this. One suggestion is to move all type tests into a *.test-d-ts type test and make use of Vitest’s type assertion utilities. There are other PRs of related type bugs open (like with newer TS) that need to be solved mostly with stricter typechecks than we have in the runtime tests. That’s not a hard requirement for solving this, but it would greatly help.

@drwpow drwpow added PRs welcome PRs are welcome to solve this issue! good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project labels Aug 14, 2024
@RPGillespie6
Copy link
Author

RPGillespie6 commented Aug 19, 2024

I spent several hours trying to figure this one out because I really want it fixed. The problem I've decided is that TypeScript does not have good tooling for debugging complex types like this. I can't, for example, set a breakpoint in a debugger and step through how types are propagating through generics (if such a tool exists, let me know). So it's just a lot of trial and error and it's a nightmarish debugging experience

I got so frustrated that over the weekend I made an alternate implementation of openapi-fetch that uses code generation instead of generics like I alluded to in #1779 - I'm calling it typed-fetch. Should be API compatible with openapi-fetch minus some of the extra stuff openapi-fetch has that native fetch does not have (like middlewares). I'm sure it's missing a lot of corner cases, but so far it's pretty solid with swagger petstore and doesn't seem to suffer from weird issues like this library does. Wanted to throw it out there because I think if you were to copy the architecture (swapping out golang for TypeScript), the simplicity of it would make openapi-fetch much easier to contribute to, debug, test, and maintain because you can use a real debugger.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Straightforward problem, solvable for first-time contributors without deep knowledge of the project openapi-fetch Relevant to the openapi-fetch library PRs welcome PRs are welcome to solve this issue!
Projects
Status: Accepted
Development

No branches or pull requests

6 participants