openapi-typescript icon indicating copy to clipboard operation
openapi-typescript copied to clipboard

allOff with required returning Record<string, never>

Open leppaott opened this issue 1 year ago • 13 comments

Similar question: https://github.com/OAI/OpenAPI-Specification/issues/1870

allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
          required:
            - name
            - locationUuid

This one returns as type Info & Record<string, never>

We still can't migrate from 5.4.1 version where this worked fine... All validation on fastify/Ajv work fine with this although I don't know whether it's strictly legal syntax but surely something the openAPI needs because references are useless without allowing overriding required values for request bodys versus responses etc...

leppaott avatar Dec 04 '23 12:12 leppaott

related to https://github.com/drwpow/openapi-typescript/issues/1441 as well

leppaott avatar Dec 04 '23 13:12 leppaott

Is there any movement on this? Currently affecting our application

CalebJamesStevens avatar May 30 '24 17:05 CalebJamesStevens

Yeah this is one papercut that’s frequently appeared in other issues. On the one hand, in your example we can probably all agree the intended behavior is “apply the overrides from the 2nd item to the 1st item in the array.”

But since TS doesn’t have an easy syntax for that, and it’s interpreted as an intersection, it gets a bit messy trying to translate that. To achieve that we’d have to rewrite the object from scratch, which may break in some more complex cases (what about deeply-nested $refs? in what order do the overrides apply—is it in array order, or are $refs deprioritized? how are conflicts handled when they are fundamentally different? etc).

For all these reasons, this library just tries to do as literal an interpretation to TS as possible that doesn’t involve building a new schema object from scratch so that a) there’s less interpretation from your schema -> TS, and b) there are fewer bugs in compiling types. And the current recommended workaround is just to use compositions that don’t rely on “overrides” (and just redeclare types if the shape is different) (see related docs on oneOf).

Though I’m not sure why it’s working in 5.4.1 🤔 it’s likely a tradeoff happening because maybe this generates correctly for your usecase, but unions & intersections (and how TS handles them) has broken for many people in versions < 6 and there were improvements made to most schemas (and 7.x has even more improvements there). Apologies that this is just one area where translating more “TypeScript-friendly” types in many areas caused a regression in allOf. To that end, I’d welcome any PRs that improve this behavior if it doesn’t cause breaking changes.

drwpow avatar Jun 03 '24 19:06 drwpow

@drwpow yeap doesn't seem 5.4.1 is correct to remove the optionality but at least allowed accessing those values. Generated something like:

(components['schemas']['Info'] &
          ({
            nested?: components['schemas']['Nested'] &
              ({ [key: string]: unknown } & {
                field: unknown;
              });
          } & {
            name: unknown;
          }))[];

Having the unknown type here seems why TS skips the "override".

Because in this example:

type A = {
  name?: string;
  nested: { nest?: string };
};

type B = {
  name: string;
  nested: { nest: string };
};

type Result = A & B;
const a: Result = {} as unknown as Result;
a.name; // string
a.nested.nest; // string

TS sees both fields as required so order of A/B doesn't seem to related, but TS chooses the strictest type.

So seems this example should work out of box if the tool transpiled it into

allOf:
        - $ref: '#/components/schemas/Info'
        - type: object
          properties:
             name:
                type: string
             locationUuid:
                type: string
          required:
            - name
            - locationUuid

Would mapping this kind of override without properties but only required be possible? That's a workaround I'll have to try otherwise I guess as you can't reference properties easily themselves. WIll duplicate property definitions in many places but overall think it's better than to redeclare a type multiple types for different required fields? AWithName AWithNameAndLanguage...

leppaott avatar Jun 05 '24 06:06 leppaott

@leppaott right you’re thinking about it exactly right, and your example would work I believe. But the problem is when the properties don’t match, or are incompatible. OpenAPI doesn’t place any restrictions on what those 2 types could be, so there are many scenarios where the tool couldn’t generate the type.

Perhaps we could just fall back to the current output if it’s not possible to generate types, but I’d also be scared this highly-conditional output would also be confusing. I understand the current solution may not work for everyone, but I do believe “just generating TS as directly as possible, without building synthetic types” does cover the widest usecases because then in all scenarios this tool generates something, and the limitation is of TS moreso than this tool.

For a little extra context, in previous versions this tool has also output more custom types like an XOR type rather than a TS union (which isn’t exclusive), but I’ve found trying to fight against TS and “outsmart it” usually breaks faster than just leaning into TS (especially because TS has improved so quickly over the years, and what may be a bugfix in one version may be a regression in the next). So though it’s not explicitly-stated, this project does seek to produce the most direct translation to TS, and unblock the user by letting them make small adjustments their schema knowing it will always generate TS, and that’s usually a better experience than waiting on a bugfix

drwpow avatar Jun 05 '24 12:06 drwpow

How about if on 3.1 references allow sibling keywords:

B:
  $ref: '#/components/schemas/A'
  required:
   - field2

Seems more technically correct, not overriding but adding to A? Should it apply to TS types too? This definitely limits the edge cases not allowing incompatible types?

So I assume this is a don't fix? We can modify our schemas freely, but there's always that if some OpenAPI generators generate schemas like this too. Would be nice to have a flag to drop e.g. the Record<string, never> to make it work more likely. Hmm I have to try if this was a blocker as seems at least on current TS this intersection is ignored? Have to see again which exactly broke.

Any input others @CalebJamesStevens

leppaott avatar Jun 06 '24 06:06 leppaott

This issue is stale because it has been open for 90 days with no activity. If there is no activity in the next 7 days, the issue will be closed.

github-actions[bot] avatar Sep 05 '24 02:09 github-actions[bot]

The issue still stands in my mind. The issue seems to be https://stackoverflow.com/questions/69002560/intersection-type-with-recordstring-never-does-not-allow-property-access i.e. you can read values from these types but assigning to a type with such an intersection e.g. Info & Record<string, never> is impossible without losing type-safety. Might be a TS issue but still wonder is that Record<string,never> the right type...

Does replacing that with & {} break something. Essentially providing straightforward generation and usable types - even though fields would still be optional despite trying to override with required.

leppaott avatar Sep 06 '24 10:09 leppaott