-
-
Notifications
You must be signed in to change notification settings - Fork 532
allOff with required returning Record<string, never> #1474
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
Comments
related to #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 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 |
@drwpow yeap doesn't seem 5.4.1 is correct to remove the optionality but at least allowed accessing those values. Generated something like:
Having the unknown type here seems why TS skips the "override". Because in this example:
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
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? |
@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:
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 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. Does replacing that with |
@leppaott I think openapi-typescript's (and typescript's) behaviour in regard to
Because 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 Solutions
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 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:
- $ref: '#/components/schemas/Info'
- required:
- name
- locationUuid would generate WithRequired<Info | unknown, 'name' | 'locationUuid'>;
// instead of just
Info | unknown; Nested 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 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 👍 I guess this can be closed then if there's a way to correct API to fix this. |
Won't work if you are composing required fields from refs though. Just an example: components:
barSchema:
type: object
properties:
a:
type: string
barRequiredFields:
required: ["a"]
barFullyRequired:
allOf:
- $ref: '#/components/barSchema'
- $ref: '#/components/barRequiredFields' We have these a lot. Our use case is that we use the same schema definitions for GET and PUT/PATCH, only that in GET we need all properties required, in PUT/PATCH they are optional and not required. And on top of that, our objects extend each other, so we have a list of required fields in the base object that all others re-use. I'm just curious, could you not just interpret any Like foo:
type: object
properties:
a:
type: string
required: [a, b] would yield interface: interface foo {
a: string;
b: unknown
} The intersection behavior of typescript should work fine this way: fooSchema:
type: object
properties:
foo:
type: string
fooRequired:
required: [foo] |
Also this:
Generates something like |
Similar question: OAI/OpenAPI-Specification#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...The text was updated successfully, but these errors were encountered: