-
-
Notifications
You must be signed in to change notification settings - Fork 531
str:str dictionary generates str:str|undefined dictionary in v7.0.0 #1732
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
I think this is a duplicate of #1727? |
Seems related for sure. But I don't understand how this is desired behavior. I also wonder how it relates to issue #1018 where it seems things were moving in the direction of removing I am no Typescript expert, but the intention here is to declare a dictionary, that maps str to str. Not to anything else. It is type checked at the source (Python backend in this case) to ensure that is true. class SupportedLanguages(BaseModel):
languages: dict[str, str] That then becomes an openAPI spec: "SupportedLanguages": {
"properties": {
"languages": {
"additionalProperties": {
"type": "string"
},
"type": "object",
"title": "Languages"
}
},
"type": "object",
"required": [
"languages"
],
"title": "SupportedLanguages"
} Which is the way to describe a dictionary in OpenAPI AFAIK. No dictionary has all the keys, obviously. But in my case, if it has a key it maps to a string. In fact, in my Typescript code I declare the array that holds the languages as languages: {
[key: string]: string;
}; The intention was to declare a dictionary that maps certain strings to strings (not to null or undefined). This is now incompatible with the type opeanapi-typescript 7.0.0 creates. This declaration makes What is the suggested fix or best practice in this case? Declare all Typescript dictionaries with |
Strong agree with @WarpedPixel. I had to stay on version 6 because of this. I think the intent vs. the implementation might be confused. I understand that it's an effort to prevent you from assuming a key exists. But simply checking for the existence of the key does not remove the Contrast that with what the schema is actually declaring, which is that if a key exists, then here are the only allowed values. |
Reposting from another issue: Given the feedback on this, I think it’s probably better to reverse-course and match v6 implementation. I think the reasoning I gave may have been from even before v6, and the rewrite may have accidentally regressed this behavior. I’ll open a PR to reverse this behavior in a patch, and let TypeScript do the handling. This gets really close to a “breaking change,” but given the specific nature of how this works, removing | undefined shouldn’t actually break anything as far as I’m aware of; it should just fix the opened issues. However, if we did the opposite—add that | undefined, that would be a breaking change. |
Ah the plot thickens—there actually are times/places in which we do need to add permissions: {
issues?: string;
[key: string]: string;
}; This is actually invalid—it should be It’s worth noting that all the generated examples pass TS checks, and simply removing this behavior causes this library to generate invalid TS. So we’ll have to take a more surgical approach to this, and generate whatever TS wants to try and satisfy the rest of the union. |
Description
A dictionary of string -> string defined in the openapi.json spec, generates a string -> string | undefined dictionary in Typescript. This worked as expected with 6.7.7.
openapi-typescript
7.0.0
v20.14.0
macOS 14.5
Reproduction
Excerpts from
openapi.json
Expected result
Output from 6.7.7:
Actual result
Output from 7.0.0:
Checklist
npx @redocly/cli@latest lint
)The text was updated successfully, but these errors were encountered: