Open leppaott opened 11 months ago
related to https://github.com/drwpow/openapi-typescript/issues/1441 as well
Is there any movement on this? Currently affecting our application
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 $ref
s? in what order do the overrides apply—is it in array order, or are $ref
s 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 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 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
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
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.
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 I think openapi-typescript's (and typescript's) behaviour in regard to Record<string, never>
in your case is probably correct and consistent given that
additionalProperties: false
for objects
if not specified (ie Record<string, never>
)allOf
is independently verifiedBecause of 1, your example is interpreted by openapi-typescript as
allOf:
- $ref: '#/components/schemas/Info'
- type: object
+ additionalProperties: false
required:
- name
- locationUuid
Because of 2 there is nothing that is valid according to that schema (the properties are required, but not allowed since they aren't defined in the same schema object and are not allowed as additional properties). So it's reasonable, even desirable and expected, that openapi-typescript generates a type that you can't assign anything to.
There are a few solutions to the Record<string, never>
problem (see below).\
openapi-typecript won't mark the properties required though, so it won't generate the type that you're really after, but at least you'll be able to express it in your openapi spec, while generating a valid type. Then you can use something like type-fest's SetRequired on top of the types generated by openapi-typescript to make the properties required on the type as well.
UPDATE: Just do
allOf:
- $ref: '#/components/schemas/Info'
required:
- name
- locationUuid
Seems to work in both openapi 3.0.x (tested rendering with swagger and redoc) and openapi-typescript
@drwpow I came across this issue while looking for a way to override the required properties for a schema. This is a common use case for us because we have endpoints that support a "draft" object with many optional properties, while other endpoints use the same object but with many of the same properties required. We're using openapi v3.0.x.
I'm wondering if it might be possible to use WithRequired
to support it for allOf
?
I'm not sure if it's possible, but it wouldn't require making any fundamental changes to the objects (no "rewrite from scratch"), just need to keep a context of all the required properties at the top level of the allOf
and apply that to the result of the allOf
, eg:
allOf:
- $ref: '#/components/schemas/Info'
- required:
- name
- locationUuid
would generate
WithRequired<Info | unknown, 'name' | 'locationUuid'>;
// instead of just
Info | unknown;
Nested $ref
s and allOf
s shouldn't add any complications because required
only applies to the top level properties of an object schema, so they would be in a separate context.
The key question seems to me whether it's possible to access and track the required (top level) properties for each of the schemas in the allOf
?
UPDATE: Found that I could use
allOf:
- $ref: '#/components/schemas/Info'
required:
- name
- locationUuid
Which seems to work for openapi v3.0.x both in openapi-typescript and openapi (tested both on swagger ui, redoc, and https://www.jsonschemavalidator.net/) 🎉
I think it still would be nice to support the other ways of doing this, but probably worth documenting that this is the way to do it, unless I've just missed it in the documentation.
@sebastian-fredriksson-bernholtz good find if it works :+1: I guess this can be closed then if there's a way to correct API to fix this.
Similar question: https://github.com/OAI/OpenAPI-Specification/issues/1870
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...